Skip to content
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

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented Dec 8, 2020

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:

  • 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)
  • Applying the env vars doesn't work with out export (thanks for explaining export vs. set @sbwsg !)
  • 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

/kind documentation

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • [n/a] Includes tests (if functionality changed/added)
  • [n/a] Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

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

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/documentation Categorizes issue or PR as related to documentation. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 8, 2020

```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/
Copy link
Contributor

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.

Copy link
Collaborator Author

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-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2020
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
Copy link
Member

@pritidesai pritidesai Dec 9, 2020

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.)
Copy link
Member

@pritidesai pritidesai Dec 9, 2020

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.

Copy link
Collaborator Author

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


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
Copy link
Member

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.

Copy link
Member

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

Copy link
Collaborator Author

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!

@afrittoli
Copy link
Member

/test check-pr-has-kind-label

Copy link

@ghost ghost left a 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.


```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
Copy link

@ghost ghost Dec 9, 2020

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.

Copy link
Collaborator Author

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?

Copy link

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! 👍

Copy link
Collaborator Author

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_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
Copy link

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.

Copy link
Collaborator Author

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.:

  1. I run the command to set the var:
TEKTON_RELEASE_GIT_RESOURCE="foo"
  1. I run env to see what environment variables are set, it's not listed

  2. I export it instead:

 export TEKTON_RELEASE_GIT_RESOURCE="foo"
  1. 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!

Copy link

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.

Copy link
Collaborator Author

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 processexport 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 !

Copy link
Collaborator Author

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

Copy link

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!


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).
Copy link
Collaborator Author

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

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
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 5, 2021
```

```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
Copy link
Collaborator Author

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... 😅 )

Copy link

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.

Copy link

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)

Copy link
Collaborator Author

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

@tekton-robot
Copy link
Collaborator

[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:
  • OWNERS [dlorenc,sbwsg,vdemeester]

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

@ghost
Copy link

ghost commented Jan 5, 2021

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.
@ghost
Copy link

ghost commented Jan 6, 2021

Given that this has three approves I'm going to switch mine to an lgtm. 🚀

/lgtm

@tekton-robot tekton-robot assigned ghost Jan 6, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2021
@tekton-robot tekton-robot merged commit af39b0f into tektoncd:master Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants