-
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
Add a release pipeline and update the release docs 📖 #1095
Add a release pipeline and update the release docs 📖 #1095
Conversation
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.
Super cool! I think we could consider this as fixing #531 as well (it doesn't have the GitHub part yet but we'll get to that!)
I left a bunch of minor comments :D biggest thing is I'm not sure about making ppl (manually) make a new PipelineResource with a new name for each release, seems like it can be error prone and is kinda complicated to explain and I'm not sure its worth it (i think ideally we'd have the randomized resource names #685 and/or embed the resources #544 )
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/lint.yaml | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/build.yaml | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/tests.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.
😍
``` | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/lint.yaml | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/build.yaml | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/tests.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.
We can keep discussing this (I don't want to block merging this!) but I'm not totally sold on running all the tests as part of the release pipeline. The tests are run on the code merged with HEAD before merging, so if one of these tests fails it would mean that either 1) something we are not accounting for changed (e.g. a dependency that's being pulled in) or 2) something is flakey. I think 99.9% of the time it would be the second case, in which case the tests would just slow down the release process + result in having to run it again. I guess we would be doing this to try to catch (1)? I would rather try to catch that by running continuous tests than doing it at release time 🤔
What are your thoughts on this @vdemeester ? (Maybe this would catch something else I haven't thought of - or maybe it's just worth it for the increased confidence!)
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.
So overall I do agree with you but there is one case that is not clear yet. I'll took 0.5.2 to explain this one — as we cherry-picked commits on the branch, we didn't do pull-request, and thus we did not run the entire test suites on it — that's mainly why I did #1088.
That's why I felt safe to run tests during the release (even if, there, it's only the unit tests 😅).
Honestly I am ok not running them but I do like the idea that if it fails because of flakyness:
- it will make us prioritize fixing that flakyness 😝
- it slows the release but by a tiny tiny amount of time (a matter of minutes 😝)
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.
For me the main point of doing release time testing would be doing extra testing there that is normally too time/resource consuming to run on each commit/PR - I'm not sure we have such tests (yet?) though. I guess we need to see how long the upgrade testing will take to run once it's been written.
Another way to run those kind of tests is to have a periodic (daily?) run of those tests; the issue I've seen in the past with periodic tests is that is hard to have people putting effort in keeping them green - since they don't block merging patches - but if we use such tests for pre-release it might give the extra motivation needed.
Kind of diverging from the main topic, periodic testing, if kept green, have an extra value, which is to provide a source of "clean" data to detect flaky test. Test jobs on PR may fail because the code in the PR makes them fail, however periodic tests should be 100% passing, so failures there are good data for identifying and troubleshooting flaky tests.
This said, I would not mind keeping the unit tests in there anyways.
|
||
- You can use [`tkn`](https://github.com/tkn/cli) to run the [release | ||
pipeline](./release-pipeline.yaml) (see [Creating a new | ||
release](#creating-a-new-release)) |
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.
🎉
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.
Whoops I think https://github.com/tkn/cli is wrong?
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.
Lol 😂
tekton/README.md
Outdated
- name: url | ||
value: https://github.com/tektoncd/pipeline # REPLACE with your own fork | ||
- name: revision | ||
value: v0.5.2 # REPLACE with your own commit |
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.
in the example id rather include a version that is clearly invalid just in case someone accidentally copies + pastes this (and overwrites an actual release 😅 #983)
--resource=builtWebhookImage=webhook-image \ | ||
--resource=builtDigestExporterImage=digest-exporter-image \ | ||
--resource=builtPullRequestInitImage=pull-request-init-image \ | ||
pipeline-release |
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.
😻
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 can't wait until we can create those resources via the command line too! :D
tekton/resources.yaml
Outdated
apiVersion: tekton.dev/v1alpha1 | ||
kind: PipelineResource | ||
metadata: | ||
name: tekton-pipelines-v0-5-2 |
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 think naming this after a particular release will make it harder or more confusing to reuse this, can we give this a generic name instead?
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.
otherwise our instructions will need to tell ppl to create a new resource with the right name which imo is more complication than it's worth?
tekton/release-pipeline-run.yaml
Outdated
resources: | ||
- name: source-repo | ||
resourceRef: | ||
name: tekton-pipelines-v0.X.Y |
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 think the name of the resource is actually tekton-pipelines-v0.5.2 right now?
tekton/README.md
Outdated
tkn pipeline start \ | ||
--param=versionTag=${VERSION_TAG} \ | ||
--serviceaccount=release-right-meow \ | ||
--resource=source-repo=${SOURCE_REPO} \ |
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 have some comments below - not sure it's worth making a new pipelineresource (manually) for each version?
Really cool, thank you! I've not done a deep review yet, I'll try to do so as soon as possible. Not for this PR, but one thing that we need to do in future is test upgrades with resources, so that we can catch issues like the one with the default timeout i.e.:
|
@afrittoli see #1114 for that 👼 |
1a7cf95
to
3e81325
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 added a bunch of minor comments, but nothing that blocking really, so I would be happy for this version to be merged, and then we can update on it if needed.
You will need to set the following parameters: | ||
- `versionTag`: to set the tag to use for published images | ||
|
||
**TODO(#983) Be careful! if you use a tag that has already been released, you |
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 we actually check that? We can fail by default on existing tag, and introduce an optional pipeline parameter force
to allow overriding a release in the unlikely case we want to do that - which we never want to do anyways.
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.
Definitely would prefer this to be automatic! #983
# delete the previous run and resources | ||
|
||
# Apply golang tasks from the catalog | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/lint.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.
NIT: we might have an environment variable to optionally run the release in a dedicated namespace
|
||
tkn pipeline start \ | ||
--param=versionTag=${VERSION_TAG} \ | ||
--serviceaccount=release-right-meow \ |
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.
NIT: Having the service account as an env variable would make it easier to copy/paste this command when running in a local cluster. Unless you want to promote everyone creating a release-right-meow
service account, which would be a valid goal to aim for 😸
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/build.yaml | ||
kubectl apply -f https://raw.githubusercontent.com/tektoncd/catalog/master/golang/tests.yaml | ||
|
||
# Apply the publish Task | ||
kubectl apply -f 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.
Why do we run the catalog tasks from github directly, and the tekton ones from local file-system? Those tasks should be executed on the same reference that is in the git resource, or we should mention that one has to locally check-out the version that we are releasing.
|
||
# If you are running this in a shared cluster, delete the pipelinerun | ||
# Create the resoruces | ||
kubectl apply -f tekton/resources.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 resources need to be customized (git reference for sure, tekton-bucket might be customized when running a "personal" release.
description: package to release | ||
default: github.com/tektoncd/pipeline | ||
- name: imageRegistry | ||
default: gcr.io/tekton-releases # REPLACE with your own registry |
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.
NIT: I don't think we need to replace the default value here. Setting it in the pipelinerun is enough.
- name: source | ||
resource: source-repo | ||
- name: unit-tests | ||
runAfter: [lint] |
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.
Why does this have to run after lint?
- name: source | ||
resource: source-repo | ||
- name: build | ||
runAfter: [lint] |
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.
why run after lint?
- name: source | ||
resource: source-repo | ||
- name: publish-images | ||
runAfter: [build, unit-tests] |
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 think we could add lint here to the list, if any of the three fails we do not publish images
- name: url | ||
value: https://github.com/tektoncd/pipeline # REPLACE with your own fork | ||
- name: revision | ||
value: vX.Y.Z-invalid-tags-boouuhhh # REPLACE with your own commit |
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.
NIT: what about having a fake sha1 as an example? Or are we saying we should create the git tag first?
I think that as long as we do testing before releasing the order should be:
- run this pipeline
- if successful, create a git tag
The alternative could be to create minor versions if test fails. Or we could change the pipeline to:
- run tests
- publish a git tag
- publish images
Even though hopefully in future we can fully automate this, and when we create a new tag run this pipeline - assuming github runs a webhook for that, but I I would assume it does.
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.
Just a quick ping to this @vdemeester since we're coming up to @dibyom being about to roll out a new release :D
- adds a release pipeline (`release-pipeline.yaml`) - removes the ci-* that are already in the `golang`'s task from the catalog - update the `tekton/README.md` doc to refer those changes, givin an example of using `tkn` for creating the release-pipeline run. Signed-off-by: Vincent Demeester <[email protected]>
3e81325
to
b0193b6
Compare
[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 |
/lgtm |
/test pull-tekton-pipeline-integration-tests |
Changes
Now that
v0.5.2
is released, let's update thetekton/*
docs with what I used to do the release.release-pipeline.yaml
)golang
's task from the catalogtekton/README.md
doc to refer those changes, givin anexample of using
tkn
for creating the release-pipeline run./assign @bobcatfish
Signed-off-by: Vincent Demeester [email protected]
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task and TaskRun to build and release this image