Skip to content
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

Use Configuration.get_default_copy #567

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

clive-jevons
Copy link
Contributor

What do these changes do?

When loading the config from the load_incluster_config or load_kube_config methods
the result is stored in Configuration._default. We must then use the get_default_copy
method in order to retrieve a copy of the configuration containing the actually
config values previously loaded. Testest with kubernetes==12.0.0

Description

When creating a new Configuration instance using the constructor, the host always defaulted to http://localhost which resulted in the client not being able to connect if this is not what is actually configured in the kubeconfig.

With this change the loaded config is retrieved via the get_default_copy which then contains the configured values.

Only tested with kubernetes==12.0.0.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • [x ] The code addresses only the mentioned problem, and this problem only
  • [x ] I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

When loading the config from the load_incluster_config or load_kube_config methods
the result is stored in Configuration._default. We must then use the get_default_copy
method in order to retrieve a copy of the configuration containing the actually
config values previously loaded. Testest with kubernetes==12.0.0
@clive-jevons clive-jevons requested a review from nolar as a code owner October 20, 2020 13:33
@nolar
Copy link
Owner

nolar commented Oct 20, 2020

Some extra information for history:

The method was added to kubernetes here:

And used there too:

This commit has been released in kubernetes==12.0.0 the earliest, there are no older versions with it. The release was done 6 days ago (2020-10-15).

@nolar
Copy link
Owner

nolar commented Oct 20, 2020

@clive-jevons Thanks for the contribution!

Assuming that Kopf does not keep kubernetes as a requirement in setup.py (on purpose: it is not "required", but only "used" if and when it is installed), what would be the best way to ensure that this method exists in the installed version of the client library in different setups/apps? E.g. if people have kubernetes==11.0.0 installed or pinned, and upgrade to kopf==0.29, where this PR is merged and released, which breaks the compatibility.

One option that I see would be to detect the version of kubernetes at runtime (not sure what is the easiest way), and warn/fail if it is <12.0.0. Another option is to check for the existence of the method and use it only if present, fall back to the class creation if absent.

What do you think?

@nolar nolar added the enhancement New feature or request label Oct 20, 2020
@clive-jevons
Copy link
Contributor Author

Agreed. And I think the checking for the method approach is probably best. Have adapted the PR accordingly 😁

@nolar nolar merged commit abf98e2 into nolar:master Oct 20, 2020
@nolar
Copy link
Owner

nolar commented Oct 20, 2020

Thanks! Merged. It will be released in late Oct or early Nov approximately (I'm now beautifying the main feature for the 0.29 release).

@clive-jevons
Copy link
Contributor Author

Brilliant, thank you 🙏🏻😊

@clive-jevons clive-jevons deleted the use-kubernetes-default-config branch October 20, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants