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

docs: cleanup and tidy up the 1.11 upgrade guide #18093

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

aanm
Copy link
Member

@aanm aanm commented Dec 2, 2021

This upgrade guide contained all other versions in it. To prevent users
from mistakenly reading an old upgrade guide, we should remove those
leftovers.

Signed-off-by: André Martins andre@cilium.io

@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.11 labels Dec 2, 2021
@aanm aanm requested a review from a team as a code owner December 2, 2021 03:09
@aanm aanm requested a review from qmonnet December 2, 2021 03:09
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.0 Dec 2, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

RST change/cleanup looks good.

Are you sure we should delete the instructions for older versions?

  • How will people find the documentation they need if they want to update from an older version? Should we at least point them to the older versions of the documentation?
  • This list is also sometimes useful for developers, for tracking the evolution of configuration options for example. Now we'll have to go through the git history or to checkout an older branch to find it?

Would it make sense to keep this information but to “hide” it on another page that doesn't get as much exposure, and to link to it from the upgrade guide?

Note that the rest of the document (the Advanced section) also has a few references to older versions, see for example https://docs.cilium.io/en/latest/operations/upgrade/#migrating-from-kvstore-backed-identities-to-kubernetes-crd-backed-identities Beginning with cilium 1.6...

@aanm
Copy link
Member Author

aanm commented Dec 2, 2021

RST change/cleanup looks good.

Are you sure we should delete the instructions for older versions?

It's up for debate. I think it doesn't make sense to have all of that text for older versions since we only support upgrades from a minor version to the next minor version.

  • How will people find the documentation they need if they want to update from an older version? Should we at least point them to the older versions of the documentation?

Each documentation is specific for a branch, they can find the upgrade from 1.9 to 1.10 in the v1.10 branch

  • This list is also sometimes useful for developers, for tracking the evolution of configuration options for example. Now we'll have to go through the git history or to checkout an older branch to find it?

🤔 I never came across that myself. Can you give a specific example?

Would it make sense to keep this information but to “hide” it on another page that doesn't get as much exposure, and to link to it from the upgrade guide?

Note that the rest of the document (the Advanced section) also has a few references to older versions, see for example https://docs.cilium.io/en/latest/operations/upgrade/#migrating-from-kvstore-backed-identities-to-kubernetes-crd-backed-identities Beginning with cilium 1.6...

Correct, although it is available since Cilium 1.6, users can still opt-in to migrate from kvstore to crd in 1.10/1.11/1.12...

@qmonnet
Copy link
Member

qmonnet commented Dec 2, 2021

It's up for debate. I think it doesn't make sense to have all of that text for older versions since we only support upgrades from a minor version to the next minor version.

I understand and I'm not opposed to have them removed from that page, but at the same time I'm not sure it should disappear altogether from the repository.

  • How will people find the documentation...

Each documentation is specific for a branch, they can find the upgrade from 1.9 to 1.10 in the v1.10 branch

I know that, but I'm not sure that all users know. This is why I suggested that we should at least replace the removed content with a paragraph pointing to those documentation branches (If you seek to upgrade from an older version, see [the older versions of this document](...)...)

  • This list is also sometimes useful for developers, ...

thinking I never came across that myself. Can you give a specific example?

The example I had in mind was about troubleshooting a user issue, with an older version of Cilium involved, and we realised that the bug was due to a configuration mistake: The user was relying on the former version of an option which had since changed name. I read the upgrade guide to find this out.

If we remove the older guides from this branch I will still be able to read the former upgrade notes from the older branches of the online documentation, but it will be less convenient than reading them locally from the .rst file in my opinion. Then if we keep removing the legacy notes for each new release, in time, I will have to go through all the existing branches if I'm looking for a deprecation and I don't know in what version it was removed.

Note that the rest of the document...

Correct, although it is available since Cilium 1.6, users can still opt-in to migrate from kvstore to crd in 1.10/1.11/1.12...

👍

(Overall I'm not strongly opposed to removing the older notes, it does make the guide cleaner - I'm simply voicing concerns that I believe we should at least consider before proceeding.)

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

This is a nice cleanup.

In the past I was maybe more inclined to keep these here for reasons similar to what Quentin raises above, but this doc is really big and we don't really intend for OSS users to upgrade directly across multiple minor releases so it should be enough to just link the older guides and encourage upgrading through each minor release.

As for figuring out that users have specified an unsupported configuration item, I wonder if it'd be better to just come up with a simple config validator tool that could highlight the problems. Ultimately if the user specifies foo in the helm charts and expects that cilium-agent gets configured with --foo, then the result should be fairly easy to cross-reference with either the logs at the agent startup or vs. cilium config --all to determine whether the setting is set correctly. From there, it should be easy to bisect whether the problem is in helm or in the agent.

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
Documentation/operations/upgrade.rst Show resolved Hide resolved
This upgrade guide contained all other versions in it. To prevent users
from mistakenly reading an old upgrade guide, we should remove those
leftovers.

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 3, 2021
@nathanjsweet nathanjsweet merged commit ce68d37 into cilium:master Dec 3, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.0 Dec 3, 2021
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Dec 5, 2021
@joestringer joestringer moved this from Backport pending to v1.11 to Backport done to v1.11 in 1.11.0 Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.11.0
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

None yet

4 participants