-
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 some more release process details 🤓 #3614
Conversation
tekton/release-cheat-sheet.md
Outdated
|
||
```bash | ||
# This will update your kubectl to automatically connect to the dogfood cluster. | ||
# Remember to point it back to your own cluster afterward, or configure acesss | ||
# to multiple clusters: https://kubernetes.io/docs/tasks/access-application-cluster/configure-access-multiple-clusters/ |
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 don't think the steps you linked to are really an "or". The gcloud command does exactly that - it fetches the auth information, creates a new cluster entry in your kubectl, and then also swaps the current cluster over to that one. You need to do that the first time you use the dogfood cluster, but you can use normal kubectl config commands every time after that.
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.
kk ill try to make this more accurate
tekton/release-cheat-sheet.md
Outdated
TEKTON_VERSION=# Example: v0.11.2 | ||
TEKTON_RELEASE_GIT_RESOURCE=# Example: tekton-pipelines-v0-11-2 | ||
TEKTON_IMAGE_REGISTRY=gcr.io/tekton-releases | ||
export TEKTON_VERSION=# Example: v0.11.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.
since you are updating this line, how about pointing it to a new release instead of patch i.e. replacing .2
with .0
in v0.11.2
|
||
9. The YAMLs are now released! Anyone installing Tekton Pipelines will now get the new version. Time to create a new GitHub release announcement: | ||
1. Choose a name for the new release! The usual pattern is "< cat breed > < famous robot >" e.g. "Ragdoll Norby". | ||
(Check the previous releases to avoid repetition: https://github.com/tektoncd/pipeline/releases.) |
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 listed all release names in a gist here 😱 I think it will be very helpful to make it part of the repo/website somewhere, just the table, instead of going through more than dozen different release pages.
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.
oh wow that would be really handy to refer to XD
tekton/release-cheat-sheet.md
Outdated
|
||
5. On successful completion, a URL will be logged. Visit that URL and sort the | ||
1. On successful completion, a URL will be logged. Visit that URL and sort the |
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 do have to manually add Upgrade and deprecation notices, I am not 100% sure though.
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.
that's true, I manually added those notices today
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'll add this to the list!
/test check-pr-has-kind-label |
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'd like to push back a bit on any PRs that are introducing multiline comments without associated listed actions to the cheat-sheet. If we really want a doc with exposition / prose then I'd like to suggest we bring back the long-form deployment guide we used to have. Totally my own POV but I much prefer to keep these kinds of docs as focused, concise and accurate as possible.
tekton/release-cheat-sheet.md
Outdated
|
||
```bash | ||
# This will update your kubectl to automatically connect to the dogfood cluster. | ||
# Remember to point it back to your own cluster afterward, or configure acesss |
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'd argue this comment isn't necessary and actually hinders the "cheat-sheet"-ness here. The line before this comment says "Configure kubectl
to use the dogfooding cluster" which is pretty much the same thing as "This will update your kubectl to connect to the dogfood cluster". And then the comment about changing back to your own cluster is a dedicated listed action at the end of this cheat-sheet which tells the user to "Stop pointing kubectl
at dogfooding cluster." with the kubectl command to use.
Generally speaking in a "cheat-sheet" this kind of commentary tends to slow me down rather than help me, and in addition it's asking me to hold something in my working memory while I go through a bunch of other steps, which is a little antithetical to the doc's purpose. Linking out to "moar docs" to read is kinda the whole reason I wrote this in reaction to our previous long deployment guide.
If the action at the end which describes how to switch back to your own cluster isn't enough I suggest updating it to be more helpful rather than add exposition in earlier sections.
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.
If the action at the end which describes how to switch back to your own cluster isn't enough I suggest updating it to be more helpful rather than add exposition in earlier sections.
Personally I find that my strict adherence to the steps in the cheat sheet wanes the further I get down the list - and maybe more importantly, if something interrupts me halfway through, this would leave me in a dangerous state where hopefully I get to the last item in the cheat sheet list but if I don't, I might modify the CI cluster
it's asking me to hold something in my working memory while I go through a bunch of other steps
I feel like its worse to ask you to definitely complete all the steps and hold in your working memory that you are by default connected to a "production" cluster 😬
@sbwsg what if we default to using the --context
arg in the instructions? maybe we could assume you have a --context dogfooding
context configured or similar, and link to some docs on how to set that up?
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.
@sbwsg what if we default to using the --context arg in the instructions? maybe we could assume you have a --context dogfooding context configured or similar, and link to some docs on how to set that up?
Seems like a reasonable compromise! 👍
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.
okay updated @sbwsg lemme know what you think!
tekton/release-cheat-sheet.md
Outdated
TEKTON_IMAGE_REGISTRY=gcr.io/tekton-releases | ||
export TEKTON_VERSION=# Example: v0.11.2 | ||
export TEKTON_RELEASE_GIT_RESOURCE=# Name of the resource you created, e.g.: tekton-pipelines-v0-11-2 | ||
export TEKTON_IMAGE_REGISTRY=gcr.io/tekton-releases # only change if you want to publish to a different 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.
Which step further down failed when you didn't include the export
? FWIW I don't think I've ever had to use them and it shouldn't be necessary unless (for example) I opened a tmux after setting these envs, or otherwise somehow created a subprocess shell before using them.
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 my environment (zsh with iterm on a mac) a line like TEKTON_RELEASE_GIT_RESOURCE="foo"
is only applicable for the running command (process?), e.g. something like TEKTON_RELEASE_GIT_RESOURCE="foo" mycommand
but isn't retained for future commands unless exported, e.g.:
- I run the command to set the var:
TEKTON_RELEASE_GIT_RESOURCE="foo"
-
I run
env
to see what environment variables are set, it's not listed -
I export it instead:
export TEKTON_RELEASE_GIT_RESOURCE="foo"
- Now when I run
env
I see it set:
env
...
TEKTON_RELEASE_GIT_RESOURCE=foo
_=/usr/bin/env
I think this explains it pretty well: https://stackoverflow.com/questions/7328223/unix-export-command
If you set a variable at the command-line like
$ FOO="bar"
That variable will not be visible in child processes
I kinda wonder how it's working with your setup!
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'm running the same shell and OS. It's true that it won't show up with env
because env
only shows variables marked for export. Run set
and you'll see it listed.
Here's a tiny example to test it out:
$ FOO=bar
$ echo $FOO
This prints "bar" for me.
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.
ahhh okay i think im starting to understand what's happening - i think im explaining what you already understand, but for my own understanding: so if you say FOO=bar
, it will be set for the current process (TIL about set
) - if you wanted FOO to be set for any child processes, you'd need to use export
# set BAR in the current environment/process
✗ BAR=baz
✗ echo $BAR
baz
# we have a script that tries to use the same variable
✗ cat foo.sh
set -x
echo $BAR
# the variable isn't exported, so when this is run, a child process starts and is not provided with that variable
✗ ./foo.sh
+ echo
# if we export the variable, it will be provided to the child process
✗ export BAR=hello
✗ ./foo.sh
+ echo hello
hello
And in the cheatsheet, we're resolving the variables in the shell that runs the commands, so no need to pass them to any child processes - so I think I'm on board now with not including export
- lemme know if my understanding isn't correct @sbwsg !
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 I've made this same change in other docs 🤦♀️ I'll try to reverse it when I see it. Thanks again @sbwsg , I feel like my overall understanding is deeper now :D
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.
This matches my understanding yeah!
tekton/release-cheat-sheet.md
Outdated
|
||
15. Update [the catalog repo](https://github.com/tektoncd/catalog) test infrastructure | ||
1. Update [the catalog repo](https://github.com/tektoncd/catalog) test infrastructure | ||
to use the new release by updating the `RELEASE_YAML` link in [e2e-tests.sh](https://github.com/tektoncd/catalog/blob/v1beta1/test/e2e-tests.sh). |
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.
@jerop just noticed this link is using the wrong branch
976af5f
to
eb37a21
Compare
Worked with @jerop to create the v0.19.0 release today and there were a few parts of the cheat sheet that felt like they could use a bit more detail: - You can configure kubectl to connect to multiple clusters if you want - Some info on how to choose the commit to release - The step that applies the PipelineResource was applying a file in the tekton dir that probably wasnt the one you meant to apply (the old instructions directed ppl to edit that file) - Changed the numbering in markdown to all be `1.` so you can add more steps without having to update all the numbers - Added some guidance on naming the release. - Mentioned that you have to uncheck the pre-release box (which I didn't realize last time XD) - Don't use patch versions in the example
eb37a21
to
a0aadee
Compare
tekton/release-cheat-sheet.md
Outdated
``` | ||
|
||
```bash | ||
# Test backport | ||
kubectl apply --filename https://storage.googleapis.com/tekton-releases/pipeline/previous/v0.11.2/release.yaml | ||
kubectl --context dogfooding apply --filename https://storage.googleapis.com/tekton-releases/pipeline/previous/v0.11.2/release.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.
@sbwsg @afrittoli is the idea that you'd run these tests directly against the dogfood cluster, or against your own cluster? (i have to admit ive always skipped this step... 😅 )
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 feel like "test" is, in hindsight, maybe too strong a word here - all I do is apply the new release to my own cluster and watch the deployments in the tekton namespace to make sure that the controller comes up successfully with the new version.
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.
(but yeah, sorry, to answer the question - apply them to your own cluster, not to dogfooding)
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.
ahhh kk, I'll change this to use the my-dev-cluster context then :D
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc, sbwsg, 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 |
Looks great, thanks for making these changes! |
This change is trying to balance a few things: * If you've never done a release before, the cheat sheet is useful but doesn't have all the info you might need. This change is trying to give new folks enough to be able to make changes against the right cluster but also not bog down the "cheat sheet" nature of the list * If you connect to the dogfooding cluster, by default all kubectl (and tkn) commands will be run against it, which can be dangerous if you forget So this change updates the examples to assume you have a --context setup that you can use to easily refer to the dogfooding cluster and includes some very brief instructions at the bottom of the doc on how to set this up.
68a3b3d
to
912d49e
Compare
Given that this has three approves I'm going to switch mine to an lgtm. 🚀 /lgtm |
Changes
Worked with @jerop to create the v0.19.0 release today and there were a
few parts of the cheat sheet that felt like they could use a bit more detail:
tekton dir that probably wasnt the one you meant to apply (the old
instructions directed ppl to edit that file)
Applying the env vars doesn't work with out(thanks for explaining export vs. set @sbwsg !)export
1.
so you can add moresteps without having to update all the numbers
realize last time XD)
/kind documentation
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 to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes