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

fix(kubernetes): let kubectl handle namespaces #208

Merged
merged 5 commits into from
Feb 12, 2020
Merged

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Feb 11, 2020

Instead of naively setting the default namespaces on objects that don't
have one, we now overlay $KUBECONFIG with a patch to set the default
namespace on the context, so that kubectl will do the job for us.

kubectl does this far more intelligent than we did, especially it does
not inject the namespace on objects that don't take one.

Fixes #190

There is one TODO left in injecting the patch into $KUBECONFIG:

for i, s := range env {
// TODO: handle empty $KUBECONFIG
if strings.HasPrefix(s, "KUBECONFIG=") {
env[i] = fmt.Sprintf("KUBECONFIG=%s:%s", k.nsPatch, strings.TrimPrefix(s, "KUBECONFIG="))
}

Instead of naively setting the default namespaces on objects that don't
have one, we now overlay `$KUBECONFIG` with a patch to set the default
namespace on the context, so that `kubectl` will do the job for us.

`kubectl` does this far more intelligent than we did, especially it does
not inject the namespace on objects that don't take one anymore.
@sh0rez sh0rez added kind/bug Something isn't working component/kubernetes Working with Kubernetes clusters labels Feb 11, 2020
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is a good approach. Just one comment about handling the lack of $KUBECONFIG now rather than having to do it later.

pkg/kubernetes/client/kubectl.go Outdated Show resolved Hide resolved
Properly handles the implicit default namespace (`""`, missing field) in
`client.DiffServerSide`.

Before, these objects were flagged as non-existent and always displayed
a diff, which was incorrect.

Adds a test to ensure this for the future
@sh0rez
Copy link
Member Author

sh0rez commented Feb 12, 2020

Fixed an issue with native always returning a diff for things on the default namespace.

Added tests:

  • separating objects in missing namespace and ready during native diff (old functionality, broke here)
  • injecting $KUBECONFIG

PTAL @rfratto

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sh0rez sh0rez merged commit 499e102 into master Feb 12, 2020
@sh0rez sh0rez deleted the default-namespace branch February 12, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/kubernetes Working with Kubernetes clusters kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tanka incorrectly specifies a namespace when creating a Namespace
2 participants