-
Notifications
You must be signed in to change notification settings - Fork 1.5k
KEP-3104: promote kuberc to beta #5300
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh 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 |
@@ -261,6 +262,12 @@ As a user I would like to be able to opt out of deprecation warnings. | |||
|
|||
https://github.com/kubernetes/kubectl/issues/1317 | |||
|
|||
#### Story 5 |
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.
Do you think lock file is a prerequisite for beta promotion?
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.
This use case seems a little strange to me, are there other examples of behavior like this for customizations / aliases in other projects?
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, once we remove this story. This PR is ready for PRR.
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.
Discussed this with @mpuckett159 and we agreed to drop this.
@@ -305,21 +313,32 @@ required) or even code snippets. If there's any ambiguity about HOW your | |||
proposal will be implemented, this is the place to discuss them. | |||
--> | |||
|
|||
During alpha this feature will be enabled through the `KUBECTL_KUBERC=true` environment variable. The file will default to being located in `~/.kube/kuberc`. A flag will allow overriding this default location with a path i.e. `kubectl --kuberc /var/kube/rc`. | |||
For beta this feature will be enabled by default. However users can disable it | |||
setting the `KUBECTL_KUBERC` environment variable to `false`. |
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.
Users can also disable this functionality via KUBERC=off
anytime regardless of beta, ga, alpha, etc.
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.
Updated.
@@ -369,7 +388,11 @@ Based on reviewers feedback describe what additional tests need to be added prio | |||
implementing this enhancement to ensure the enhancements have also solid foundations. | |||
--> | |||
|
|||
Aside from standard testing we will also be skew testing. | |||
We're planning to expand tests adding: |
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.
all tests as a prerequisite for 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.
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, we don't promote to beta and enable by default without all planned tests in place
https://groups.google.com/a/kubernetes.io/g/dev/c/thuIea5aYtw
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.
So if we add - config API fuzzy tests
and input validation and correctness
in here, this will be requirement for beta promotion.
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.
That is correct, and that's why kubernetes/kubernetes#131818 contains all the necessary tests. Although, more will likely followup.
- [x] (R) Production readiness review completed | ||
- [x] (R) Production readiness review approved | ||
- [x] "Implementation History" section is up-to-date for milestone | ||
- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] | ||
- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes | ||
- [x] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] |
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.
Why is this checked? I don't see docs for kuberc.
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.
+1 for expecting more docs / examples in place at this point... looks like only a single envvar and mention in the blog was added for alpha
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 with you. Probably @soltysh marked this due the same reason as well. But isn't this something needed closer to the end of release cycle?. testing + required code pieces -> promotion to beta -> doc changes
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.
doc changes for alpha → beta, sure
but docs for usage / examples should have already been added for alpha... that needs to be backfilled
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.
Where is the exact location that usage and examples need to be updated?
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 would say at minimum, we need:
- API docs for the kuberc file format, similar to https://kubernetes.io/docs/reference/config-api/kubeconfig.v1/
- A reference from https://kubernetes.io/docs/reference/kubectl/quick-reference/ about kuberc (either inline or a link to a dedicated kuberc page)
- A reference from the "Optional kubectl configurations and plugins" section on the kubectl setup pages (either inline or a link to a dedicated kuberc page)
We spent a long time on the API docs of the go types for the kuberc page. Rendering those out to an annotated exhaustive reference (sort of like https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#webhook-request-and-response does for admission types) would be good, along with several examples / recipes of how to use the kuberc file.
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.
Opened this kubernetes/website#50913
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.
Given the above in-flight PR, and followup which will expand on the beta elements I'm going to leave this checkbox as is.
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 has genie-out-of-bottle consequences for the K8s ecosystem.
I recommend not even merging the code to promote to beta until there are GA quality docs that explain the alpha of this mechanism.
Wait, GA quality? Why skip beta?
For docs, especially about things on by default, there is no difference. [With exceptions] beta features should have documentation of just as high quality as GA, and promotion to stable should be trivial for the docs element.
I would like us to hold on this change until we have a plan to make the config format easier to teach. |
The next iteration of the changes I'm currently working on should address this. But I'm fine holding on with merging this to ensure all the bits are addressed. |
@@ -295,6 +295,7 @@ Consider including folks who also work outside the SIG or subproject. | |||
| Risk | Impact | Mitigation | | |||
| --- | --- | --- | | |||
| Confusing users with a new config file | Low | Documentation and education | | |||
| Users trust the kuberc is being used | High | Provide mechanism warning users default kuberc is not being picked up | |
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.
Do we need to remove this?.
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 do, that was added with that story 5 removed. Good catch, thank you.
- More rigorous forms of testing—e.g., downgrade tests and scalability tests | ||
- All functionality completed | ||
- All security enforcement completed | ||
- All monitoring requirements completed |
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 this is here as required?, because monitoring is not applicable for this 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.
This is here from the recently updated template. For kubectl the monitoring is handled through the verbosity, which we resolved recently in kubernetes/kubernetes#131668
Signed-off-by: Maciej Szulik <[email protected]>
@ardaguclu updated |
/lgtm This is ready for PRR |
/assign @ardaguclu
for sig-cli review
/assign @johnbelamaric
for PRR review
/hold
so it doesn't merge accidentally before I get both of the above reviews in