-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Document a breaking change in the default configuration #1303
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
Document a breaking change in the default configuration #1303
Conversation
CHANGELOG.md
Outdated
@@ -24,6 +24,9 @@ Kubernetes API Version: 1.16.14 | |||
|
|||
Kubernetes API Version: 1.16.14 | |||
|
|||
**Breaking Change:** | |||
- `kubernetes.config.Configuration()` no longer returns the default configuration. Use `kubernetes.config.Configuration.get_default_copy()` now to get the default configuration. [OpenAPITools/openapi-generator#4485](https://github.com/OpenAPITools/openapi-generator/pull/4485), [OpenAPITools/openapi-generator#5315](https://github.com/OpenAPITools/openapi-generator/pull/5315) |
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.
it's a bit confusing ...
kubernetes.config.Configuration() still returns the default "initial" configuration,
kubernetes.config.Configuration.get_default_copy() will return the default configuration if there is a default set via Configuration.set_default(c), otherwise, it will also return the default "initial" configuration
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. I reworded it a little bit, since the change in kubernetes.config.Configuration()
was the regression.
82a4310
to
d845af2
Compare
/lgtm |
I was wondering if we should do this as a minor release since changelog and releases are usually immutable. /hold |
@palnabarun Yes, I think that will help raise awareness. Would you mind fold the change to #1285, and add an action required note in 12.0.1? |
Yeah sure. Let me cherry-pick your commit and apply there. |
@roycaihw Can you please split your changes to the CHANGELOG and tests into two commits? I want to preserve attribution when I edit those. :) |
The test addition also requires adding a note to the Hot Patch section to the release docs. We need to specify that the commit which adds the test needs to be cherrypicked while performing the release. |
d845af2
to
6ae4721
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: roycaihw, yliaog 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 |
@palnabarun Thanks. I split the commit into two |
/close in favor of #1285 |
@roycaihw: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
xref: #1284, OpenAPITools/openapi-generator#4485, OpenAPITools/openapi-generator#5315
Document a breaking change of
kubernetes.config.Configuration()
that exists since v12.0.0a1. Add a unit test that asserts the behavior.Special notes:
Configuration()
to easily access one global configuration.Configuration()
is _refresh_oidc. The client doesn't rely on the default configuration.