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: correctly set namespace when checking for an existing helm release via skaffold deploy #3914

Merged
merged 4 commits into from Apr 7, 2020

Conversation

mrparkers
Copy link
Contributor

Fixes: #3838

Description

When using skaffold deploy with helm, skaffold was not honoring the --namespace command-line flag when checking if a helm release already exists. You can reproduce this error using the code within the examples/helm-deployment folder:

Before this PR:

cd examples/helm-deployment
kubectl create ns skaffold-test
skaffold build -q | skaffold deploy --namespace skaffold-test --build-artifacts - # results in a successful deployment
skaffold build -q | skaffold deploy --namespace skaffold-test --build-artifacts - # results in the error "cannot re-use a name that is still in use"

After this PR:

cd examples/helm-deployment
kubectl create ns skaffold-test
skaffold build -q | skaffold deploy --namespace skaffold-test --build-artifacts - # results in a successful deployment
skaffold build -q | skaffold deploy --namespace skaffold-test --build-artifacts - # results in a successful helm upgrade

@mrparkers
Copy link
Contributor Author

cc @clement-buchart

@mrparkers
Copy link
Contributor Author

side note: please correct me if I'm wrong, but it looked like a lot of the assertions for the existing unit tests around the helm deploy code were incorrect - most of them were asserting that the --namespace flag was not used in the first helm get, but was used in the following helm upgrade. this didn't make sense to me, I think that we should be expecting that the --namespace flag is used in both cases or in neither case.

I went ahead and fixed these tests and added a few more that used a namespaced skaffold context to assert that the --namespace flag is being used when the skaffold context contains a namespace, even if the helm release does not contain a namespace.

please let me know if I misunderstood something here and if I have to revert it back to the way it was.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #3914 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/helm.go 78.80% <91.66%> (+1.03%) ⬆️
pkg/skaffold/server/server.go 58.57% <0.00%> (ø)

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Apr 7, 2020
@dgageot dgageot self-assigned this Apr 7, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 7, 2020
@dgageot
Copy link
Contributor

dgageot commented Apr 7, 2020

Awesome! Thanks a lot @mrparkers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants