-
Notifications
You must be signed in to change notification settings - Fork 195
Elastic Stack Helm chart upgrade and configuration changes documentation #4388
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
Open
eedugon
wants to merge
5
commits into
main
Choose a base branch
from
eck_helm_upgrades
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+147
−9
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7905046
Elastic Stack Helm chart upgrade and configuration changes
eedugon 7df2eb4
Merge branch 'main' of github.com:elastic/docs-content into eck_helm_…
eedugon c2d99aa
linting hint
eedugon f93faef
Apply suggestions from code review
eedugon 004dff7
Merge remote-tracking branch 'origin/main' into eck_helm_upgrades
eedugon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 note kind of works, but I don't like how it's splitting the instructions and example, and it is burying an up-front choice in the middle of the procedure
consider instead:
not sure if there is a better way to word the headings, because it's less about the method of performing the upgrade, than the context of the configuration ... some alternative headings might be:
"For orchestrators using spec files"
"For orchestrators using Helm charts"
orchestrators / operators / environments / deployments / clusters ... whatever feels the most accurate
you see what I'm getting at :)
Uh oh!
There was an error while loading. Please reload this page.
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 see your point, and I agree.... however I didn't want to do extra changes on documents that are already established and feel very simple (in a good way).
Take in mind the following and let me know your thoughts.... :)
Currently we have this:
The doc intro feels accurate for all ECK users (helm vs no helm) as it starts with an intro about upgrading in general with links to preparations, etc.
Then when it comes to the "procedure" what we show is that technically upgrading is super simple, just change the
versionof your spec. It's true that we don't mention helm based installations earlier, but considering how small the procedure is and that we have the note right after I think it should be fine.I also think it's good to "force" a helm-user to read the standard procedure, because at the end of the day, what they have to achieve through helm is the same (change the
versionon the final spec file).What I think we could really to do improve this is moving upwards these two paragraphs that are currently at the end:

They are relevant to all users and those feel a big hidden at the end of a big example. Maybe those 2 paragraphs could be in the intro. What do you think? :)
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 moving up those paragraphs makes sense, but I still think that forcing helm chart users to read through this procedure is a little confusing, especially because the easiest way to upgrade is via a flag on a helm command, which might impact the underlying spec file, but it's not the same thing as editing the spec file directly.
won't block this but that is my opinion