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

Output a URL with instructions after running linkerd upgrade #2627

Merged
merged 5 commits into from
Apr 4, 2019

Conversation

scottcarol
Copy link
Contributor

Fixes #2575. When a user successfully runs linkerd upgrade in the CLI, they will see a message after the yaml output pointing to a URL with full upgrade instructions.

@scottcarol scottcarol added area/cli priority/P0 Release Blocker labels Apr 3, 2019
@scottcarol scottcarol self-assigned this Apr 3, 2019
@scottcarol scottcarol requested review from ihcsim and alpeb April 3, 2019 01:05
@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 3, 2019

Integration test results for 805e230: success 🎉
Log output: https://gist.github.com/25e98b9edd7c9f503ed3bfa94ee25bfd

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Two comments below.

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
@alpeb
Copy link
Member

alpeb commented Apr 3, 2019

What do you all think about appending an anchor with the version into that url? We could link directly to the upgrade notice for that version (if any). The downside is that if the user is upgrading from a much older version they'll require to check not only the current upgrade notice but also previous ones.

@grampelberg
Copy link
Contributor

@alpeb sounds great to me. I've got an outstanding issue to start versioning the docs, so we could use the anchor to redirect to the correct version.

@scottcarol
Copy link
Contributor Author

@grampelberg @alpeb I like the ⚓️ idea. I separated them out into success and failure, would that work with the doc architecture?
https://linkerd.io/upgrade/#nextsteps
https://linkerd.io/upgrade/#troubleshooting

Re: specific version numbers, when running linkerd upgrade can you specify version numbers? Or will you always get the latest upgrade?

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 3, 2019

Integration test results for 7f22e2f: success 🎉
Log output: https://gist.github.com/9129d25f4c91aa12b8acc2f703c38c1c

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Show resolved Hide resolved
@ihcsim
Copy link
Contributor

ihcsim commented Apr 4, 2019

Re: specific version numbers, when running linkerd upgrade can you specify version numbers? Or will you always get the latest upgrade?

It looks like I can decide which version number to upgrade/downgrade to with linkerd upgrade --linkerd-version=edge-19.3.3

$ linkerd upgrade --linkerd-version edge-19.3.3
...
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  annotations:
    linkerd.io/created-by: linkerd/cli edge-19.4.1 # original
  creationTimestamp: null
  labels:
    linkerd.io/control-plane-component: identity
  name: linkerd-identity
  namespace: linkerd
...
    spec:
      containers:
      - args:
        - identity
        - -log-level=info
        image: gcr.io/linkerd-io/controller:edge-19.3.3 # downgrade version
...

@olix0r Does this look right to you?

Copy link
Contributor

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big overall improvement! One minor nit:

$ bin/go-run cli upgrade | kubectl apply -f -
namespace/linkerd unchanged
configmap/linkerd-config configured
serviceaccount/linkerd-identity unchanged
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-identity unchanged

√ You're on your way to upgrading Linkerd!
Visit this URL for further instructions: https://linkerd.io/upgrade/#nextsteps
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-identity unchanged
service/linkerd-identity configured
secret/linkerd-identity-issuer configured
deployment.extensions/linkerd-identity configured
serviceaccount/linkerd-controller unchanged
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-controller unchanged
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-controller unchanged
service/linkerd-controller-api configured
service/linkerd-destination configured
deployment.extensions/linkerd-controller configured
customresourcedefinition.apiextensions.k8s.io/serviceprofiles.linkerd.io configured
serviceaccount/linkerd-web unchanged
service/linkerd-web configured
deployment.extensions/linkerd-web configured
serviceaccount/linkerd-prometheus unchanged
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-prometheus unchanged
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-prometheus unchanged
service/linkerd-prometheus configured
deployment.extensions/linkerd-prometheus configured
configmap/linkerd-prometheus-config configured
serviceaccount/linkerd-grafana unchanged
service/linkerd-grafana configured
deployment.extensions/linkerd-grafana configured
configmap/linkerd-grafana-config configured

The instructions get lost in the wall of text (and loose the colors because of pipes). How about a couple newlines afterwards? Maybe a separator?

@klingerf
Copy link
Contributor

klingerf commented Apr 4, 2019

@grampelberg Interesting find. It also looks like linkerd upgrade must be writing the YAML to stdout before writing the success message to stderr, so kubectl gets a head start on printing its output before the success message shows up. @scottcarol, maybe we can update this PR to write to stderr first?

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 4, 2019

Integration test results for cc41777: success 🎉
Log output: https://gist.github.com/f272dc0a049bf74b71ccde5f9ee33a55

@scottcarol
Copy link
Contributor Author

Ok, now we're rendering values to a buffer, which prints when the render is complete. That way the okStatus string prints separately. Thanks @klingerf for the help.

(Unfortunately the colors won't print when piping to kubectl which is a Unix thing)

Outstanding question @grampelberg @ihcsim are the URLs ok? Right now they are:
https://linkerd.io/upgrade/#nextsteps
https://linkerd.io/upgrade/#troubleshooting

@grampelberg
Copy link
Contributor

URLs work for me!

@klingerf
Copy link
Contributor

klingerf commented Apr 4, 2019

Those URLs 404 for me. Should they be https://linkerd.io/2/tasks/upgrade/#upgrade-the-data-plane for success and https://linkerd.io/2/tasks/upgrade/#step-by-step-instructions for failure?

@ihcsim
Copy link
Contributor

ihcsim commented Apr 4, 2019

The URLs LGTM.

@grampelberg Are we setting URL redirects for these URLs?

@klingerf
Copy link
Contributor

klingerf commented Apr 4, 2019

Re: redirects -- hugo strips anchors when redirecting, so it won't work for https://linkerd.io/upgrade/#nextsteps to redirect to a #nextsteps anchor on a different page. We'd need to link to that page directly for the anchor to work.

By way of example, see how https://linkerd.io/2/ha/#setup redirects to https://linkerd.io/2/features/ha/, which drops the #setup anchor.

@siggy
Copy link
Member

siggy commented Apr 4, 2019

@klingerf Re: redirects, I had assumed that we could set up https://linkerd.io/upgrade/#foo links similar to the checks URLs, for example: https://linkerd.io/checks/#pre-k8s-cluster-k8s

Implemented as static HTML:
https://github.com/linkerd/website/blob/master/linkerd.io/static/checks/index.html

I've found this approach useful for linkerd check, as it creates a stable URL that's independent of any changes in hugo or the docs.

@klingerf
Copy link
Contributor

klingerf commented Apr 4, 2019

@siggy Ah, that's a good approach. I wasn't aware that we were doing non-hugo redirects. Carry on.

Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐️ Nice work! I like this approach a lot, and had just a few tiny style nits.

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
@grampelberg
Copy link
Contributor

URLs look good to me. @scottcarol do you mind adding those when this PR merges?

@scottcarol
Copy link
Contributor Author

scottcarol commented Apr 4, 2019

URLs look good to me. @scottcarol do you mind adding those when this PR merges?

@grampelberg To clarify, you're saying the URLs look good as written:

success: https://linkerd.io/upgrade/#nextsteps
failure: https://linkerd.io/upgrade/#troubleshooting

and that I should add the URL forwarding to linkerd.io after this PR merges?

Can do! There have been a lot of different URLs mentioned in this PR so I want to be sure I've got it right.

@grampelberg
Copy link
Contributor

@scottcarol yup, and yes please!

@scottcarol scottcarol requested a review from ihcsim April 4, 2019 22:54
@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 4, 2019

Integration test results for 5fc8238: success 🎉
Log output: https://gist.github.com/fd4b834e9c506a320be41ea30ee85de8

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 💯

@scottcarol scottcarol merged commit e15ce7f into master Apr 4, 2019
@scottcarol scottcarol deleted the cscott/add-upgrade-message branch April 4, 2019 23:32
KatherineMelnyk pushed a commit to KatherineMelnyk/linkerd2 that referenced this pull request Apr 5, 2019
…rd#2627)

Adds a URL to the `linkerd upgrade` output which contains full upgrade instructions. The message and the URL anchors are different in the case of success or failure.

Fixes linkerd#2575. 

Signed-off-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants