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

Adds a warning in the upgrade doc about split cluster #17755

Merged
merged 1 commit into from Nov 3, 2021

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Nov 1, 2021

See commit msg.

We saw "split" cluster (mismatching agent and operator versions) causing weird behaviors (e.g., #17192), adding a warning in upgrade guide to discourage running "split" clusters.

@Weil0ng Weil0ng requested a review from a team as a code owner November 1, 2021 23:21
@maintainer-s-little-helper
Copy link

Commit 66695fce7612badadee2bacbffc7eeb13611577a does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 1, 2021
@maintainer-s-little-helper
Copy link

Commit 66695fce7612badadee2bacbffc7eeb13611577a does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@Weil0ng Weil0ng added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 1, 2021
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.

The PR message says to look at the commit message, but the commit message is empty. Were you meaning to point out something else from the PR description?

I've added a couple of notes. It's always tempting to highlight the one extra doc piece that you are submitting as a developer in either a .. note:: or .. warning:: box, but if everyone does this then we don't end up with a flowing document that describes the functionality of the software, we just end up with a series of notes and warnings. While this is important, I don't think that we need to raise specific attention to it in a warning box; it's enough to document it as part of the guide as standard text.

@@ -4,6 +4,12 @@
Please use the official rendered version released here:
https://docs.cilium.io

.. warning::
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the preview, I think we would ideally integrate this text into the guide somewhere a bit more naturally rather than as the first warning that appears on the page. While this is an important point, I don't think that it needs to be the first thing on the page and it probably doesn't need to be called out in a warning box. The upgrade guide has barely started at this point and this would mean there are already two warnings 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Curious how did you get the preview line? (I tested the change locally via make render-docs)

Copy link
Member

Choose a reason for hiding this comment

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

The link? We haven't had this for too long, but if you scroll down to the bottom in the checks, there is one called deploy/netlify. If you click on "Details" there then it'll provide a preview of the PR docs.

Documentation/operations/upgrade-warning.rst Outdated Show resolved Hide resolved
Signed-off-by: Weilong Cui <cuiwl@google.com>
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.

LGTM thanks.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.6 Nov 3, 2021
@joestringer joestringer merged commit 1c1c06a into cilium:master Nov 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.10.6 Nov 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Nov 23, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.6 Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.6
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

4 participants