-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow using non default context #2354
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
Conversation
|
|
|
Welcome @omarkhammassi! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omarkhammassi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| new InputStreamReader( | ||
| new FileInputStream(kubeConfig), StandardCharsets.UTF_8.name()))) { | ||
| KubeConfig kc = KubeConfig.loadKubeConfig(kubeConfigReader); | ||
| KubeConfig kc = KubeConfig.loadKubeConfig(kubeConfigReader, context); |
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.
Don't change this signature, just call kc.setContext(...) after the kube config is created.
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 keep the parameter list minimal and neat
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.
Done.
| return standard(true, context); | ||
| } | ||
|
|
||
| public static ClientBuilder standard(boolean persistConfig, String context) throws IOException { |
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.
Rather than change this signature, I think we should add a context(String context) method to ClientBuilder
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.
Done.
| if (context != null) { | ||
| kubeConfig.setContext(context); | ||
| } else { | ||
| String currentContext = (String) configMap.get("current-context"); |
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: promote "currrent-context" to a static final variable in this class
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.
Done.
| new InputStreamReader( | ||
| new FileInputStream(kubeConfig), StandardCharsets.UTF_8.name()))) { | ||
| KubeConfig kc = KubeConfig.loadKubeConfig(kubeConfigReader); | ||
| KubeConfig kc = KubeConfig.loadKubeConfig(kubeConfigReader, context); |
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 keep the parameter list minimal and neat
|
|
||
| public static ClientBuilder context(String context) throws IOException { | ||
| final File kubeConfig = getKubeConfigFile(); | ||
| ClientBuilder clientBuilder = getClientBuilder(true, kubeConfig, context); |
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.
Don't change the signature here, just call kubeconfig.setContext(context) before you call getClientBuilder(context)
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.
kubeConfig is of type File , setContext(context) can only be set at a later stage when building the client
| } | ||
|
|
||
| private static ClientBuilder getClientBuilder(boolean persistConfig, File kubeConfig) | ||
| private static File getKubeConfigFile() { |
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 messy. We should really refactor the whole ClientBuilder class so that it defers all of this construction until the build() method is called.
That's beyond the scope of this PR though (unless you want to :)) but please add a TODO: defer construction to build() method here.
| } | ||
|
|
||
| public static ClientBuilder context(String context) throws IOException { | ||
| final File kubeConfig = getKubeConfigFile(); |
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 isn't quite what I was expecting, I think that we should support ClientBuilder.standard().context("foo").build() as well as ClientBuilder.kubeconfig(someFile).context("foo").build()
I think what this means is that we need to save off the KubeConfig object in the builder rather than constructing it new each time.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
Closing this PR as stale, please feel free to re-open it if you plan to work on it further. |
This PR adds this ability to specify which context to try to create configuration from.