Use value from ContextDetails to populate Namespace #373
Conversation
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @rynowak! |
k8s-ci-robot
added
the
size/M
|
I signed it |
k8s-ci-robot
added
cncf-cla: yes
|
/assign @brendandburns |
| // This serves as validation for any of the properties of ContextDetails being set. | ||
| // Other locations in code assume that ContextDetails is non-null. | ||
| throw new KubeConfigException($"Cluster not set for context `{currentContext}` in kubeconfig"); | ||
| } |
There was a problem hiding this comment.
Added this validation because I see a bunch of code running after this at assumes activeContext.ContextDetails is null, and it will crash with a null-ref based on invalid config data.
This is a fix for: kubernetes-client#372 This change uses the value from ContextDetails.Namespace to populate KubernetesClientConfiguration.Namespace. The issue is there's a Namespace property on both Context and ContextDetails - The property on Context is used today - The property on ContextDetails is not - The property on ContextDetails maps to the actual yaml config
This property doesn't map to anything in the YAML and thus will never be
set. Other clients I checked (java, golang) don't look for a property
at this level.
I think this was likely a mistake, and it should be obsoleted because
it will never be populated.
Example:
```yaml
contexts:
- context:
cluster: ...
namespace: ... # this is ContextDetails.Namespace
user: ...
name: foo
```
```yaml
contexts:
- context:
cluster: ...
namespace: ...
user: ...
name: foo
namespace: ... # this is Context.Namespace
```
rynowak
force-pushed
the
rynowak/fix-372
branch
from
e774afe
to
e687129
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
/LGTM |
k8s-ci-robot
added
the
lgtm
|
/lgtm Thanks for the PR! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, rynowak 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 |
k8s-ci-robot
added
the
approved
Use value from ContextDetails to populate Namespace
Fixes: #372
This change uses the value from ContextDetails.Namespace to populate
KubernetesClientConfiguration.Namespace.
The issue is there's a Namespace property on both Context and ContextDetails
Obsolete Context.Namespace
I did a separate commit for this so we can skip it if you don't want it. Reasonable people disagree on whether its good semver to obsolete something in a patch/minor release. I'd like some confirmation as well about whether this property is supposed to be there for some reason
This property doesn't map to anything in the YAML and thus will never be
set. Other clients I checked (java, golang) don't look for a property
at this level.
I think this was likely a mistake, and it should be obsoleted because
it will never be populated.
Example: