-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate params in pipelines #7930
Propagate params in pipelines #7930
Conversation
Thanks for the release-note! |
aa47541
to
fd78e36
Compare
Done. PTAL again 🙏 Thanks! |
ebd497d
to
e5d5c27
Compare
/test pull-tekton-pipeline-alpha-integration-tests |
/test pull-tekton-pipeline-go-coverage-df |
@chitrangpatel: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
e5d5c27
to
21e510f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree with the issue #7901 to be an extension of the capability of the existing API.
While this PR generally lgtm, shall we also update the TEP0107 to reflect so?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JeromeJu 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:
Approvers can indicate their approval by writing |
On second thought - do we need an integration test for this? |
I threw in an example test for this. If that's not ok, I can work on an integration test too. The trick is that it needs us to apply a pipeline separately on the cluster. The web hook was the one stopping its admission. I already added unit tests for that. The example test also allows us to test the web hook admission more easily. It also provides a nice example :) The controller logic has not changed at all I think so the |
Happy to do that! |
/hold cc @afrittoli I put a hold so that we can all be on the same page here. Let's discuss it on the issue or in the API WG on Monday or offline. |
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good!
Could you add a "negative" test to see that propagation on a referenced task is not allowed?
Prior to this, we allowed parameter propagation in an inlined pipelinerun. However, within a pipeline, we requrie a verbose spec. This was an oversight as indicated in tektoncd#7901. This PR fixes that issue by updating the validation logic in the webhook. Fixes tektoncd#7901. Propagate params in pipelines Prior to this, we allowed parameter propagation in an inlined pipelinerun. However, within a pipeline, we requrie a verbose spec. This was an oversight as indicated in tektoncd#7901. This PR fixes that issue by updating the validation logic in the webhook. Fixes tektoncd#7901.
21e510f
to
c43d14d
Compare
Done! PTAL! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
/lgtm
Prior to this, we allowed parameter propagation in an inlined pipelinerun. However, within a pipeline, we requrie a verbose spec. This was an oversight as indicated in #7901.
This PR fixes that issue by updating the validation logic in the webhook.
Fixes #7901.
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind bug