-
Notifications
You must be signed in to change notification settings - Fork 804
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
Remove deprecated logic and emit clear messages #2211
Remove deprecated logic and emit clear messages #2211
Conversation
Verification of deprecation logicThis deprecation logic could fail in various ways, for example by the schema validation erroring before the NOTES.txt provides a more helpful message on how to migrate the configuration. Due to this, I've used the following to test these changes. Everything seems okay.
|
@yuvipanda @manics I've updated the PR summary to be a bit quicker and easier to read. This should be part of 1.0.0. |
For clarity what do you think about changing |
@manics absolutely - hard deprecation is not easy to understand or well defined! Perhaps we want to write a title of "BREAKING CHANGES", and then we write individual messages with "REMOVED" for example? Btw, do you agree on this understanding of the word "Deprecated" - do we use that word whenever we mean something that still will remain functional but is an option that we mark as something that users should transition away from? |
For me "deprecated" always means it still works for existing uses, but you probably shouldn't use it for anything new, and should consider updating existing uses to a replacement. Adding an adjective in front conveys (in an ill-defined manner since there's no standard terminology 😃) how soon you should plan to move to something else. |
@manics okay updated! See #2211 (comment) for a rendered output of the breaking changes failure message. I made changes to make the warning messages stand out visually, and here is such output rendered.
|
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, but will give @maincs a chance to merge based on prior review.
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.
A couple of very minor suggestions, otherwise feel free to merge!
f1b9628
to
f30cc97
Compare
If we remove the schema entries in 2.0.0 for these keys, it will error even by just having a key defined. Due to this, we should use hasKey instead of check for truthy values.
b7fda72
to
4f28655
Compare
jupyterhub/zero-to-jupyterhub-k8s#2211 Merge pull request #2211 from consideRatio/pr/hard-deprecation
Summary
Motivation for breaking changes
Our deprecation messages are triggered by detecting deprecated values being passed, which requires them to be accepted by the schema. So by ensuring all users have stopped using deprecated values when they upgrade to 1.0.0, we can in 2.0.0 remove them from the schema.
Breaking changes
All of the breaking changes will lead to clear error messages emitted by a
fail
statement inNOTES.txt
. This happens before any communication to the k8s api-server has been made.auth
->hub.config
hub.imagePullSecret
->imagePullSecret
(new chart wide config)singleuser.cloudMetadata.enabled
->singleuser.cloudMetadata.blockWithIptables
(renamed / inverted value)singleuser.imagePullSecret
->imagePullSecret
(new chart wide config)proxy.containerSecurityContext
->proxy.chp.containerSecurityContext
(relocated)proxy.networkPolicy
->proxy.chp.networkPolicy
(relocated)proxy.pdb
->proxy.chp.networkPolicy
(relocated)hub.extraConfigMap
is now required to unset and configured usingcustom
(renamed)hub.uid
is now required to be unset and configured usinghub.containerSecurityContext.runAsUser
(coalesced into k8s native config)Closes #2157 by providing more visually standing out messages for warnings and breaking changes.
Closes #1412 by making NOTES.txt failures grouped into a single message, which I think is sufficient for now.