--dry-run Option for tkn pipeline start #643

danielhelfand · 2020-01-27T16:46:56Z

Closes #372

Changes

This pull request adds a --dry-run option for tkn pipeline start. The idea would be to print the pipelinerun to stdout so a user can look over the pipelinerun without actually running the pipeline.

An --output option will also be made available to output the dry run in yaml or json:

Output the pipeline named pipelinename in namespace ns in yaml:

tkn pipeline start pipelinename -n ns --dry-run --output yaml

Output the pipeline named pipelinename in namespace ns in json:
tkn pipeline start pipelinename -n ns --dry-run -o json

Not using output flag with --dry-run uses yaml output default:

tkn pipeline start pipelinename -n ns --dry-run

Example out:

output = json

{
	"kind": "PipelineRun",
	"apiVersion": "tekton.dev/v1alpha1",
	"metadata": {
		"generateName": "deploy-pipeline-run-",
		"namespace": "default",
		"creationTimestamp": null
	},
	"spec": {
		"pipelineRef": {
			"name": "deploy-pipeline"
		},
		"resources": [
			{
				"name": "app-git",
				"resourceRef": {
					"name": "nodejs-ex-git"
				}
			},
			{
				"name": "app-image",
				"resourceRef": {
					"name": "nodejs-ex-image"
				}
			}
		],
		"serviceAccountName": "default",
		"podTemplate": {}
	},
	"status": {}
}

output = yaml

kind: PipelineRun
metadata:
  creationTimestamp: null
  generateName: deploy-pipeline-run-
  namespace: default
spec:
  pipelineRef:
    name: deploy-pipeline
  podTemplate: {}
  resources:
  - name: app-git
    resourceRef:
      name: nodejs-ex-git
  - name: app-image
    resourceRef:
      name: nodejs-ex-image
  serviceAccountName: default
status: {}

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Regenerate the manpages and docs with make docs and make man if needed.
Run the code checkers with make check
Commit messages follow commit message best practices

Release Notes

Add --dry-run option to tkn pipeline start to preview a pipelinerun

tekton-robot · 2020-01-27T16:49:25Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	68.7%	-1.4

danielhelfand · 2020-01-27T16:50:01Z

pkg/cmd/pipeline/start.go

@@ -376,6 +381,10 @@ func getOptionsByType(resources resourceOptionsFilter, restype string) []string

 func (opt *startOptions) startPipeline(pName string) error {
 	pr := &v1alpha1.PipelineRun{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "tekton.dev/v1alpha1",


How to specify the APIVersion seems difficult. This could maybe be pulled from the pipeline?

I think we would want at least to have it in a global since that string is scattered everywhere, but that's a refactoring that can be done for later....

🤔 I think it's fine for now, but indeed, good question. I would guess we default to a hard coded version, and we could have some smart code that checks what api version are available in the cluster (and adjust from it or error out from there)

danielhelfand · 2020-01-27T16:50:59Z

pkg/cmd/pipeline/start.go

+			return err
+		}
+
+		if err := ioutil.WriteFile(opt.DryRun, prBytes, os.ModePerm); err != nil {


I do not have unit tests for this since it writes a local file. Would it make sense to write the file and then delete it as part of the unit test?

not sure if that's the proper way but that's how i would do it 😅

Sure, I'll try it.

Wondering if this approach is the right way here, but there are at least unit tests for errors/marshaling the pipelinerun.

chmouel · 2020-01-27T16:51:33Z

I like the idea! thinking about it, what do you think about -o yaml -f file.yaml for this? as -o json may be useful too...

danielhelfand · 2020-01-27T17:01:06Z

I like the idea! thinking about it, what do you think about -o yaml -f file.yaml for this? as -o json may be useful too...

🤔 Yeah, I could see this. So potentially convert --dry-run to a boolean and specify the outputs with -o and -f. Maybe default to yaml if nothing is specified?

tekton-robot · 2020-01-27T20:02:57Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	70.5%	0.3

tekton-robot · 2020-01-27T20:04:01Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	70.5%	0.3

tekton-robot · 2020-01-27T20:31:28Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	70.6%	0.4

tekton-robot · 2020-01-28T01:09:27Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	70.6%	0.4

vdemeester

Any reason to have the -f ? Doesn't tkn pipeline start --dry-run -o yaml > foo.yaml work ? (and its equivalent in Powershell).
We are using -f as input elsewhere in tkn, and I also wonder what happens we want some input someday ? I think, the initial bet is to use stdout and let other tools that can manage stdout do there thing.

vdemeester · 2020-01-28T09:36:49Z

To add more to my previous comment, I wanna be able to do something like tkn pipeline start --dry-run --output=json | sed … | kubectl apply -f - or simial things. The PR, as-is, force me to use a -f and write a file, which might not be needed.

chmouel · 2020-01-28T10:16:53Z

humm yeah make sense, sorry @danielhelfand that i made. you change it 🙇

danielhelfand · 2020-01-28T13:00:56Z

To add more to my previous comment, I wanna be able to do something like tkn pipeline start --dry-run --output=json | sed … | kubectl apply -f - or simial things. The PR, as-is, force me to use a -f and write a file, which might not be needed.

Yeah, that makes sense. I can see scenarios where you don't actually want to output a file so makes sense. Also makes it easier to write tests for.

Would tkn pipeline start --dry-run --output=json then print the pipelinerun in json format in the case when not using >?

vdemeester · 2020-01-28T13:10:53Z

Would tkn pipeline start --dry-run --output=json then print the pipelinerun in json format in the case when not using >?

Yes 👼

danielhelfand · 2020-01-28T15:12:31Z

pkg/cmd/pipeline/start_test.go

+			namespace: "",
+			input:     cs2,
+			wantError: false,
+			want: `apiVersion: tekton.dev/v1alpha1


I think these should be golden files, but I think it would be better to refactor these to golden files in a separate pr.

Yes they should be golden files 👼 Could be done now or in a follow-up, as you want 😉

Will refactor them all in a separate pr.

tekton-robot · 2020-01-28T15:12:58Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	71.4%	1.3

danielhelfand · 2020-01-28T15:17:42Z

pkg/cmd/pipeline/start.go

+	}
+
+	if format == "json" {
+		prBytes, err := json.MarshalIndent(pr, "", "\t")


Can do something besides tab for indent if there are thoughts on that.

no idea 😅
I think kubectl prints it without indent and let the user pass it through a tool for indent ?

/cc @chmouel

I guess I would like the indent here if the behavior is to print for the end user to see.

they are spaces on kubectl output or at least according to whitespace-mode in emacs

as long it's yamllint compatible i think it's fine for me!

I guess I would like the indent here if the behavior is to print for the end user to see.

It's fine either way? printed to end user or not we could do indent?

Without the indent, it's all crammed to the left like below:

{ "kind": "PipelineRun", "apiVersion": "tekton.dev/v1alpha1", "metadata": { "generateName": "deploy-pipeline-run-", "namespace": "default", "creationTimestamp": null }, "spec": { "pipelineRef": { "name": "deploy-pipeline" }, "resources": [ { "name": "app-git", "resourceRef": { "name": "nodejs-ex-git" } }, { "name": "app-image", "resourceRef": { "name": "nodejs-ex-image" } } ], "serviceAccountName": "default", "podTemplate": {} }, "status": {} }

With the indent, it's as follows:

{ "kind": "PipelineRun", "apiVersion": "tekton.dev/v1alpha1", "metadata": { "generateName": "deploy-pipeline-run-", "namespace": "default", "creationTimestamp": null }, "spec": { "pipelineRef": { "name": "deploy-pipeline" }, "resources": [ { "name": "app-git", "resourceRef": { "name": "nodejs-ex-git" } }, { "name": "app-image", "resourceRef": { "name": "nodejs-ex-image" } } ], "serviceAccountName": "default", "podTemplate": {} }, "status": {} }

I think I'll keep as is.

yes please big +1 on indents! any chanced to check if yamllint can validate it ? (i mean not a test but manually)

Yes, it works with yamllint.

vdemeester · 2020-01-28T15:32:33Z

pkg/cmd/pipeline/start.go

@@ -376,6 +381,10 @@ func getOptionsByType(resources resourceOptionsFilter, restype string) []string

 func (opt *startOptions) startPipeline(pName string) error {
 	pr := &v1alpha1.PipelineRun{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "tekton.dev/v1alpha1",


🤔 I think it's fine for now, but indeed, good question. I would guess we default to a hard coded version, and we could have some smart code that checks what api version are available in the cluster (and adjust from it or error out from there)

vdemeester · 2020-01-28T15:38:10Z

pkg/cmd/pipeline/start.go

+	}
+
+	if format == "json" {
+		prBytes, err := json.MarshalIndent(pr, "", "\t")


no idea 😅
I think kubectl prints it without indent and let the user pass it through a tool for indent ?

/cc @chmouel

vdemeester · 2020-01-28T15:38:40Z

pkg/cmd/pipeline/start_test.go

+			namespace: "",
+			input:     cs2,
+			wantError: false,
+			want: `apiVersion: tekton.dev/v1alpha1


Yes they should be golden files 👼 Could be done now or in a follow-up, as you want 😉

tekton-robot · 2020-01-28T15:38:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

tekton-robot · 2020-01-28T21:44:29Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	71.4%	1.3

tekton-robot · 2020-01-28T21:50:59Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/start.go	70.1%	71.4%	1.3

chmouel · 2020-01-29T08:50:13Z

/lgtm

Looking Good! 🤙🏽

googlebot added the cla: yes label Jan 27, 2020

tekton-robot requested review from sthaha and vdemeester January 27, 2020 16:47

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 27, 2020

danielhelfand commented Jan 27, 2020

View reviewed changes

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 27, 2020

vdemeester requested changes Jan 28, 2020

View reviewed changes

danielhelfand commented Jan 28, 2020

View reviewed changes

vdemeester approved these changes Jan 28, 2020

View reviewed changes

tekton-robot requested a review from chmouel January 28, 2020 15:38

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

--dry-run option for tkn pipeline start

45882b5

danielhelfand mentioned this pull request Jan 29, 2020

Add --dry-run Option for tkn task start #654

Closed

tekton-robot assigned chmouel Jan 29, 2020

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

tekton-robot merged commit f4f32ac into tektoncd:master Jan 29, 2020

This was referenced Jan 30, 2020

Add --dry-run Option for tkn clustertask start #659

Closed

Add --dry-run Option for tkn task start #662

Merged

Add --dry-run Option for tkn ct start #663

Merged

danielhelfand deleted the print_pipelinerun branch February 10, 2020 16:21

--dry-run Option for tkn pipeline start #643

--dry-run Option for tkn pipeline start #643

Conversation

danielhelfand commented Jan 27, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jan 27, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmouel commented Jan 27, 2020

danielhelfand commented Jan 27, 2020

tekton-robot commented Jan 27, 2020

tekton-robot commented Jan 27, 2020

tekton-robot commented Jan 27, 2020

tekton-robot commented Jan 28, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester commented Jan 28, 2020

chmouel commented Jan 28, 2020

danielhelfand commented Jan 28, 2020

vdemeester commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jan 28, 2020

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

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

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

chmouel commented Jan 29, 2020

danielhelfand commented Jan 27, 2020 •

edited

Loading