-
Notifications
You must be signed in to change notification settings - Fork 119
[ECK] Update documentation for 3.1.0 #2019
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
Conversation
This adds documentation for the feature implemented in elastic/cloud-on-k8s#8623 Merge target is `eck-3.1.0` so that we can in preparation for the next ECK release merge all pertinent docs at once to main. --------- Co-authored-by: Edu González de la Herrán <25320357+eedugon@users.noreply.github.com>
👋 doc team, I would be glad to get a first review, just to be sure I can safely merge this PR on release date. Thanks! 🙇 |
|
||
One of those exceptions is the configuration of providers as described in [advanced Agent configuration managed by Fleet](/reference/fleet/advanced-kubernetes-managed-by-fleet.md). When {{agent}} is managed by {{fleet}} and is orchestrated by ECK, the configuration of providers can simply be done through the `.spec.config` element in the Agent resource as of {applies_to}`stack: ga 8.13`: |
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.
@barkbay : what do we mean with the applies_to stack 8.13
here? That the providers configuration can be done only for Agents running 8.13
or later?
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.
One of those exceptions is the configuration of providers as described in [advanced Agent configuration managed by Fleet](/reference/fleet/advanced-kubernetes-managed-by-fleet.md). When {{agent}} is managed by {{fleet}} and is orchestrated by ECK, the configuration of providers can simply be done through the `.spec.config` element in the Agent resource as of {applies_to}`stack: ga 8.13`: | |
One of those exceptions is the configuration of providers as described in [advanced Agent configuration managed by Fleet](/reference/fleet/advanced-kubernetes-managed-by-fleet.md). Starting in stack version 8.13, if {{agent}} is managed by {{fleet}} and orchestrated by ECK, you can configure providers using the `.spec.config` element in the Agent resource: |
Possible version switching the 8.13
statement to the narrative side.
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 content was already reviewed here without notes: #1446
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.
Thanks @pebrc for the extra details!
My opinion is still the same but of course it's not a big deal. Also when we reviewed the linked PR, I think the applies_to
was added in a later commit, as otherwise I'd probably have highlighted it.
Anyway the current text and usage of the badge is all right too, so whatever you want.
cc: @shainaraskas
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.
We can totally change it to whatever makes most sense from a docs perspective.
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.
Perfect, let's allow @shainaraskas to share her thoughts for a final decision :)
Shaina, do you like the usage of the inline badge
there? I don't feel it very intuitive and I've suggested to change it to a narrative sentence, but maybe both approaches are fine.
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 agree that this is not ideal, partially because our labels don't look right in sentences.
one reason this is hard to reframe is that this is positioned as "one of these exceptions" - is this the only exception? are exceptions only valid as of 8.13?
this could get an Exceptions
subheading that has an applies label at the heading level, ideally, if it makes sense.
if that doesn't make sense, I'd go with prose inline or a note inline. we'll have to refactor it later when we have more components at our disposal, but will read better in the short term.
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 approved this PR already but some ECK 3.1 tagging should be added here before this is shipped
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.
LGTM!
Just minor suggestions for your consideration.
The applies_to
inline badge doesn't feel natural here. In this case I'd prefer to use a narrative approach, but whatever you prefer.
no worries @barkbay! Yes, we just need to change the substitutions. PR approved already. |
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.
overall this looks good. I think you've gotten most of what needs to be done for a release - sorry we weren't more proactive with sharing what needs to be done with you.
Provided a little additional feedback in the context of our cumulative docs model.
One other thing that you should check is that the upgrade instructions are still accurate in the context of 3.1. Does a 3.1 upgrade require an incremental 3.0 upgrade?
deploy-manage/deploy/cloud-on-k8s.md
Outdated
* Kubernetes 1.29-1.33 | ||
* OpenShift 4.15-4.19 | ||
* Google Kubernetes Engine (GKE), Azure Kubernetes Service (AKS), and Amazon Elastic Kubernetes Service (EKS) | ||
* Helm: {{eck_helm_minimum_version}}+ |
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.
because these docs are going to continue to be valid for 3.0, we need to keep the 3.0 compatibility in this doc present.
could do it in tabs, table, or bulleted list. quickest hack would be:
* Kubernetes 1.29-1.33 | |
* OpenShift 4.15-4.19 | |
* Google Kubernetes Engine (GKE), Azure Kubernetes Service (AKS), and Amazon Elastic Kubernetes Service (EKS) | |
* Helm: {{eck_helm_minimum_version}}+ | |
* Kubernetes: | |
* {applies_to}`eck: 3.0`: 1.28-1.32 | |
* {applies_to}`eck: 3.1`: 1.29-1.33 | |
* OpenShift | |
* {applies_to}`eck: 3.0`: 4.14-4.18 | |
* {applies_to}`eck: 3.1`: 4.15-4.19 | |
* Google Kubernetes Engine (GKE), Azure Kubernetes Service (AKS), and Amazon Elastic Kubernetes Service (EKS) | |
* Helm: {{eck_helm_minimum_version}}+ |
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.
could do it in tabs, table, or bulleted list. quickest hack would be:
I think tabs would be the most appropriate, but let's try your suggestion first.
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.
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.
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.
Any option is fine. I like both.
The list will probably not scale well when having 8-10 releases to mention, so better the tabs for that reason probably.
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.
IMHO, the tabs are not user friendly if the user knows his k8s version and wants to find out which ECK version he needs
Not a strong argument but this is already the case today. Also I can only remember users asking if the last version of K8s or OpenShift is supported by ECK. My feeling is that we're going to have to revisit this in the future, whatever our choice is today.
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.
Anyways, I created this issue: elastic/docs-builder#1570
Maybe this could be a case for it.
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 if we go with tabs, the whole list will need to go in the tabs, because it's weird to split a list between tabs and not tabs
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.
looking at the mock of the bulleted list, I think removing the colon would make it look right if you were still considering using it
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 if we go with tabs, the whole list will need to go in the tabs, because it's weird to split a list between tabs and not tabs
Makes sense, I'll do the change
|
||
One of those exceptions is the configuration of providers as described in [advanced Agent configuration managed by Fleet](/reference/fleet/advanced-kubernetes-managed-by-fleet.md). When {{agent}} is managed by {{fleet}} and is orchestrated by ECK, the configuration of providers can simply be done through the `.spec.config` element in the Agent resource as of {applies_to}`stack: ga 8.13`: |
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 agree that this is not ideal, partially because our labels don't look right in sentences.
one reason this is hard to reframe is that this is positioned as "one of these exceptions" - is this the only exception? are exceptions only valid as of 8.13?
this could get an Exceptions
subheading that has an applies label at the heading level, ideally, if it makes sense.
if that doesn't make sense, I'd go with prose inline or a note inline. we'll have to refactor it later when we have more components at our disposal, but will read better in the short term.
Fixes elastic/cloud-on-k8s#8689 --------- Co-authored-by: David Kilfoyle <41695641+kilfoyle@users.noreply.github.com> Co-authored-by: shainaraskas <58563081+shainaraskas@users.noreply.github.com>
Co-authored-by: shainaraskas <58563081+shainaraskas@users.noreply.github.com>
Thanks for your reviews @shainaraskas! No this is not needed. |
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.
Approving to unblock
It seems |
It seems it has been renamed to |
@florent-leborgne can we get some guidance on how to update it? Does it mean that we have to create 2 PRs for each ECK release? Which one should be merged first (if it matters?). |
@barkbay I think that @elastic/docs-engineering is working on making this simpler but right now yes, you have to sync with them to bump the version in docs-builder (also see https://elastic.github.io/docs-builder/contribute/release-new-version/) |
We need to update the Generally, we should update the Applies_to tags will be rendered as After we bump the version in the |
@barkbay : I've created elastic/docs-builder#1648 to bump the We can merge it as soon as we merge this PR. I'll contact you in private if case you want me to update all our ECK docs in this PR and move the usage of the legacy substitutions to the new variables. |
I think this is ready for merging, I don't see any problem with this PR in relation with the new variables. Worst thing it could happen is to find certain places in the docs where something is not properly rendered due to this, and we could apply a quick fix afterwards. |
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.
LGTM
This PR is to update the documentation for the next ECK release.
Do not merge before ECK 3.1.0 release
Related ECK release issue: https://github.com/elastic/k8s-dev/issues/538