Add CLI and Write support to Tekton Generators #564

YolandaDu1997 · 2020-06-22T19:07:20Z

Changes

This project aims at generating Tekton spec from simplified configs, which will help users bootstrap pipelines in a configurable way.

I want to make some changes to this project:

To help users interact with generators, add the CLI command show to it.
Add support for writing output to disk.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

wlynch · 2020-06-22T19:14:28Z

Looks like this is pulling in changes from #558 can you rebase so that those diffs aren't included?

Also, I think for future PRs we can stop linking to the community issue. That was primarily to create the OWNERS file.

Thanks!

generators/cmd/cli/cmd/root.go

generators/cmd/cli/cmd/show.go

generators/pkg/writer/write.go

generators/pkg/writer/write_test.go

YolandaDu1997 · 2020-06-24T02:04:10Z

I have updated the parser and it now can parse the GithubSpec. Also, I have made changes to the cli command.

wlynch

Looking good! Just minor tweaks / cleanup.

generators/cmd/cli/cmd/write.go

generators/pkg/writer/output/gen-task.yaml

generators/pkg/writer/write.go

wlynch · 2020-06-24T16:31:20Z

generators/pkg/writer/write.go

+func WriteToDisk(filename string, writer io.Writer) error {
+	file, err := os.Open(filename)
+	if err != nil {
+		return err


Not sure what these errors look like in practice. Would it be clear to users what is failing, and what they would need to do to resolve?

If not, perhaps we should wrap these errors with some additional description of what was attempted.

I added fmt.Errorf() to provide the description of errors.

dibyom · 2020-06-24T20:14:34Z

generators/pkg/generator/generate_test.go

-		t.Fatalf("error from 'GenerateTask': %v", err)
-	}
-
+	got := GenerateTask(spec)
 	if diff := cmp.Diff(got, want); diff != "" {


it should be cmp.Diff(want,got) for the printed diff to be of the form (-want/+got)

sorry I' ll fix this

YolandaDu1997 · 2020-06-25T14:53:14Z

Done.

wlynch

Almost there! Just a few cleanups / minor changes. Core logic/structure LGTM though!

generators/pkg/writer/write_test.go

generators/pkg/writer/write.go

generators/pkg/parser/parser.go

wlynch · 2020-06-25T21:03:08Z

generators/pkg/parser/parser.go

-	res := products{}
-	err := reader.Decode(&res)
+func Parse(r io.Reader) (generator.GitHubSpec, error) {
+	res := generator.GitHubSpec{}


It's generally good practice to declare vars as close to where they are used. In this case, we're declaring res, but then doing nothing with in in the next statement. This would make more sense to be moved down to where the unmarshal occurs.

I got it. But here because if ioutil.ReadAll(r) has an error, an empty struct and the error will be returned. I could change the return value to be generator.GitHubSpec{} instead of the var.

Ah, I see! SGTM then :) (either way is fine)

wlynch · 2020-06-25T21:05:28Z

generators/pkg/writer/write_test.go

+
+	got, err := ioutil.ReadFile(tempF.Name())
+	if err != nil {
+		t.Fatal(err)


Also annotate this and the other testing errors about what failed, so it's easier to see what failed from the go test logs.

dibyom · 2020-06-26T15:00:50Z

This is looking good to me! Just need to squash up the commits :)

wlynch

/lgtm

2 really minor things, then squash the commits and we'll approve! :)

wlynch · 2020-06-26T15:03:28Z

generators/pkg/parser/parser.go

-	res := products{}
-	err := reader.Decode(&res)
+func Parse(r io.Reader) (generator.GitHubSpec, error) {
+	res := generator.GitHubSpec{}


Ah, I see! SGTM then :) (either way is fine)

wlynch · 2020-06-26T15:05:31Z

generators/pkg/parser/parser.go

 	}
+	res := generator.GitHubSpec{}
+	if err = yaml.Unmarshal(spec, &res); err != nil {


Suggested change

if err = yaml.Unmarshal(spec, &res); err != nil {

if err := yaml.Unmarshal(spec, &res); err != nil {

wlynch · 2020-06-26T15:06:21Z

generators/cmd/cli/cmd/show.go

+		err := writer.WriteToDisk(filename, os.Stdout)
+		if err != nil {


Suggested change

err := writer.WriteToDisk(filename, os.Stdout)

if err != nil {

if err := writer.WriteToDisk(filename, os.Stdout); err != nil {

Didn't notice this, sorry:(

wlynch · 2020-06-26T15:09:09Z

/approve cancel

wlynch · 2020-06-26T15:10:16Z

Did not realize that the GitHub PR approve button would trigger the Prow approve 😅 It makes sense, but I don't remember it doing this before. Sorry about the noise.

This commit is to add CLI support to generators. The CLI includes the "show" command to print the generated task configuration and the 'write' to write the generated config to file. This CLI tool is to help users interact with generators.

YolandaDu1997 · 2020-06-26T16:20:36Z

Done.

dibyom

/lgtm

tekton-robot · 2020-06-26T20:04:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~generators/OWNERS~~ [dibyom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot requested review from dibyom and wlynch June 22, 2020 19:07

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 22, 2020

YolandaDu1997 closed this Jun 22, 2020

YolandaDu1997 reopened this Jun 22, 2020

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 22, 2020

wlynch suggested changes Jun 22, 2020

View reviewed changes

wlynch suggested changes Jun 24, 2020

View reviewed changes

dibyom reviewed Jun 24, 2020

View reviewed changes

wlynch suggested changes Jun 25, 2020

View reviewed changes

wlynch approved these changes Jun 26, 2020

View reviewed changes

tekton-robot assigned wlynch Jun 26, 2020

tekton-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 26, 2020

tekton-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

YolandaDu1997 force-pushed the generators-cli branch from ca6d7ae to 47b8520 Compare June 26, 2020 16:17

dibyom approved these changes Jun 26, 2020

View reviewed changes

tekton-robot assigned dibyom Jun 26, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020

tekton-robot merged commit f63e91d into tektoncd:master Jun 26, 2020

YolandaDu1997 deleted the generators-cli branch June 26, 2020 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI and Write support to Tekton Generators #564

Add CLI and Write support to Tekton Generators #564

YolandaDu1997 commented Jun 22, 2020 •

edited

Loading

wlynch commented Jun 22, 2020

YolandaDu1997 commented Jun 24, 2020

wlynch left a comment

wlynch Jun 24, 2020

YolandaDu1997 Jun 24, 2020 •

edited

Loading

dibyom Jun 24, 2020

YolandaDu1997 Jun 24, 2020

YolandaDu1997 commented Jun 25, 2020

wlynch left a comment

wlynch Jun 25, 2020

YolandaDu1997 Jun 25, 2020

wlynch Jun 26, 2020

wlynch Jun 25, 2020

dibyom commented Jun 26, 2020

wlynch left a comment

wlynch Jun 26, 2020

wlynch Jun 26, 2020

wlynch Jun 26, 2020

YolandaDu1997 Jun 26, 2020

wlynch commented Jun 26, 2020

wlynch commented Jun 26, 2020

YolandaDu1997 commented Jun 26, 2020

dibyom left a comment

tekton-robot commented Jun 26, 2020

	if err = yaml.Unmarshal(spec, &res); err != nil {
	if err := yaml.Unmarshal(spec, &res); err != nil {

		err := writer.WriteToDisk(filename, os.Stdout)
		if err != nil {

	err := writer.WriteToDisk(filename, os.Stdout)
	if err != nil {
	if err := writer.WriteToDisk(filename, os.Stdout); err != nil {

Add CLI and Write support to Tekton Generators #564

Add CLI and Write support to Tekton Generators #564

Conversation

YolandaDu1997 commented Jun 22, 2020 • edited Loading

Changes

Submitter Checklist

wlynch commented Jun 22, 2020

YolandaDu1997 commented Jun 24, 2020

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YolandaDu1997 Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YolandaDu1997 commented Jun 25, 2020

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Jun 26, 2020

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlynch commented Jun 26, 2020

wlynch commented Jun 26, 2020

YolandaDu1997 commented Jun 26, 2020

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 26, 2020

YolandaDu1997 commented Jun 22, 2020 •

edited

Loading

YolandaDu1997 Jun 24, 2020 •

edited

Loading