-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
storage: CSI inline volumes in beta #15705
storage: CSI inline volumes in beta #15705
Conversation
/hold The feature has not been promoted to beta yet. |
Deploy preview for kubernetes-io-master-staging ready! Built with commit 23be7aa https://deploy-preview-15705--kubernetes-io-master-staging.netlify.com |
@pohly 👋 Please change the base from /milestone 1.16 |
/lgtm |
/hold cancel kubernetes/kubernetes#82004 has been merged. I also changed the base to |
You should not need to update master |
@@ -1342,7 +1343,9 @@ spec: | |||
foo: bar | |||
``` | |||
|
|||
This feature requires CSIInlineVolume feature gate to be enabled: | |||
This feature requires CSIInlineVolume feature gate to be enabled. It | |||
is enabled by default in Kubernetes >= 1.16. In Kubernetes v1.15, 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.
@pohly , is line 1346 required, as the default is enabled for v1.16?
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.
Personally I prefer to keep the information about older releases if it doesn't get too long, because then users don't need to run a mental diff against older documentation to figure out the history. Some people have to work with more than just one release of Kubernetes, so that history is relevant.
That this feature gate exists is also relevant, regardless of the history and whether its enabled or disabled by default.
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.
I assume future releases of the documentation will still have that paragraph. With that in mind I changed the second sentence to "It is enabled by default starting with Kubernetes 1.16.".
The alternative, "It is enabled by default in Kubernetes 1.16", would look weird in documentation for Kubernetes 1.17...
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.
@pohly Thanks for jumping on this. Looks like the change you mentioned It is enabled by default starting with Kubernetes 1.16 is not yet in the doc.
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.
Oh, sorry, forgot the final git push
. It's there now.
@@ -1316,6 +1316,7 @@ Learn how to | |||
#### CSI ephemeral volumes | |||
|
|||
{{< feature-state for_k8s_version="v1.15" state="alpha" >}} | |||
{{< feature-state for_k8s_version="v1.16" state="beta" >}} |
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.
nit: You could remove line 1318 as the feature is now beta.
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.
Same here: I find history important.
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.
@pohly They can always reference command-line-tools-reference/feature-gates.md
for 1.15 state.
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.
hello @pohly. The best place to record the state and history of a feature/feature gate is the feature gates reference page. If needed, you could link to the reference page. I think the feature-state
shortcode is typically used to label the current state of the feature.
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.
The k8s.io site has a toggle for release (or 1.14, 1.15, etc) right?
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.
Yes
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 removed the line for alpha in 1.15.
/lgtm |
Once the 2 issues raised have been addressed, this can be merged. |
@pohly This PR is deadline for merging is Sep 9. |
As pointed out during review, this information is better captured in the feature gates reference page.
@simplytunde: please have another look, I updated the PR. |
Deploy preview for kubernetes-io-vnext-staging processing. Building with commit 5db933e https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5d765fe7859f1e0008e8aa37 |
Awesome @pohly . Thanks for all your work. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: simplytunde The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Kubernetes 1.16 graduates this feature to beta.
Related-to: #14704,
kubernetes/enhancements#596