Skip to content

Use Timeout of --last or --use-pipelinerun/--use-taskrun for Start Commands#863

Merged
tekton-robot merged 1 commit intotektoncd:masterfrom
danielhelfand:timeout_last_usepr
Apr 6, 2020
Merged

Use Timeout of --last or --use-pipelinerun/--use-taskrun for Start Commands#863
tekton-robot merged 1 commit intotektoncd:masterfrom
danielhelfand:timeout_last_usepr

Conversation

@danielhelfand
Copy link
Copy Markdown
Member

@danielhelfand danielhelfand commented Apr 3, 2020 •
edited
Loading

Closes #862
Closes #488

Changes

When using tkn p start with the --last flag or --use-pipelinerun flag, the timeout of the previous value isn't applied as described in #862. This pull request updates both tkn p start and tkn t start to use the timeout values of the previous PipelineRun or TaskRun.

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)
  • Run the code checkers with make check
  • Regenerate the manpages, docs and go formatting with make generated
  • Commit messages follow commit message best practices

Release Notes

Use timeout value with --last and --use-pipelinerun/--use-taskrun with start commands

@tekton-robot tekton-robot requested review from chmouel and sthaha April 3, 2020 12:33
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 3, 2020
vdemeester
vdemeester approved these changes Apr 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot 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 Apr 3, 2020
@chmouel
Copy link
Copy Markdown
Member

chmouel commented Apr 3, 2020

/lgtm
/approve

Thanks 👍🏼🔥

@chmouel
Copy link
Copy Markdown
Member

chmouel commented Apr 3, 2020 •
edited
Loading

I was going to ask about unit test but unit test asked for themselves 😅

@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Apr 3, 2020

I was going to ask about unit test but unit test asked for themselves 😅

Yeah, so I guess tkn task start with --timeout wasn't working before this change. So now it's showing up in unit tests. Let me update the golden files.

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2020
@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Apr 3, 2020

/hold

If we use similar testing approaches as with --dry-run, we can get some better unit testing here.

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2020
@tekton-robot 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 Apr 3, 2020
@danielhelfand
Copy link
Copy Markdown
Member Author

danielhelfand commented Apr 3, 2020

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2020
danielhelfand
danielhelfand commented Apr 3, 2020
View reviewed changes
Comment thread pkg/cmd/pipeline/start_test.go
pipeline := Command(p)
got, _ := test.ExecuteCommand(pipeline, "start", pipelineName,
"--last",
"-r=git-repo=scaffold-git",
Copy link
Copy Markdown
Member Author

@danielhelfand danielhelfand Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test for tkn p start --last wasn't really testing the feature since it overwrites many runtime values. The point of the test is to see whether the last pipelinerun's values are used, so I removed everything except --last.

vdemeester
vdemeester approved these changes Apr 6, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@tekton-robot
Copy link
Copy Markdown
Contributor

tekton-robot commented Apr 6, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chmouel, vdemeester

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot merged commit 0a727f1 into tektoncd:master Apr 6, 2020
@danielhelfand danielhelfand mentioned this pull request Aug 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdemeester vdemeester vdemeester approved these changes

@chmouel chmouel Awaiting requested review from chmouel

@sthaha sthaha Awaiting requested review from sthaha

Assignees

@vdemeester vdemeester

@chmouel chmouel

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

timeout is not respected when using use-pipelinerun or --last Way of Testing tkn task start -t

5 participants

@danielhelfand @chmouel @tekton-robot @vdemeester @googlebot