-
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
Update pipelineTasks in Release-Pipeline to use Git Resolver #6565
Conversation
Hi @jsminem. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
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.
Thank you for your PR.
The only task hosted on pipeline side is the publish one.
Everything else comes from the catalog or the plumbing repo.
We could still switch to remote resolution, but the revision is not the pipeline one.
For catalog ones, I would suggest to use the bundles, since the version of the task is then visible in the tag.
tekton/release-pipeline.yaml
Outdated
resolver: git | ||
params: | ||
- name: repo | ||
value: pipeline | ||
- name: org | ||
value: tektoncd | ||
- name: revision | ||
value: $(params.gitRevision) | ||
- name: pathInRepo | ||
value: tekton/publish.yaml |
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.
The git-clone task is defined in the catalog, not in the pipeline repo.
We could either pull it from there with the git resolver and pin it to a revision that matches the current sha of the tip of the main
branch. Alternatively we could use the bundle remote resolver and consume it, as we do for in the plumbing repo, from the bundles published to grc.io - see https://github.com/tektoncd/plumbing/blob/2b2d5d06c8ffd885945feae76318ac8a153b3c8d/tekton/ci/jobs/tekton-golang-tests.yaml#L37-L44 for an example
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 so much Andrea! This was very helpful.
As suggested, I've used the first approach to update prerelease-checks
task in the plumbing
repo and bundles for the tasks in the catalog
repo. Any feedback would be greatly appreciated! 😊🙏
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.
Should I also include package
in params? Like in the example for bundles, https://github.com/tektoncd/catalog?
params:
- name: package
value: github.com/tektoncd/pipeline
@@ -66,7 +76,16 @@ spec: | |||
- name: precheck | |||
runAfter: [git-clone] | |||
taskRef: | |||
name: prerelease-checks |
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.
The name here should not be deleted
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.
@afrittoli I'm curious why you suggested restoring taskRef.name
? I don't think we allow it in combination with taskRef.resolver
. Separately, do you think we could add tests verifying the release pipeline configuration is valid, in order to catch issues like this?
tekton/release-pipeline.yaml
Outdated
resolver: git | ||
params: | ||
- name: repo | ||
value: pipeline |
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.
The prerelease-check
comes from the plumbing repo. It is deployed together with the pipeline to the cluster when the pipeline definition is installed, so maybe we don't need to change this for now.
If you wanted to use remote resolution here, this is the URL https://github.com/tektoncd/plumbing/blob/main/tekton/resources/release/base/prerelease_checks.yaml
Thanks so much for the feedback! 🙏 |
/ok-to-test |
4ef3f3b
to
aa7ba1b
Compare
b29cf0e
to
c73bdfb
Compare
5ea3497
to
ecf0d9f
Compare
7f54dbe
to
c6101c8
Compare
…t resolver update release pipeline to use remote resolution using bundles and git resolver update release pipeline to use remote resolution using bundles and git resolver update release pipeline to use remote resolution using bundles and git resolver update release pipeline to use git resolver update release pipeline to use git resolver update release pipeline to use git resolver update release pipeline update release pipeline update release pipeline to use remote resolution using bundles and git resolver
597f8eb
to
3d55599
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.
Thank you for all the updates @jsminem, the PR looks good now!
According to "tide", you may need to rebase it though
/approve
/cc @lbernick @vdemeester FYI |
@afrittoli: GitHub didn't allow me to request PR reviews from the following users: FYI. Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, 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:
Approvers can indicate their approval by writing |
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.
/lgtm
Update
pipelineTasks
inrelease-pipeline.yaml
fileFixes #6379
/kind misc
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