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

Support configuring default namespace for kubectl/kustomize deployers #4374

Merged

Conversation

AndiDog
Copy link
Contributor

@AndiDog AndiDog commented Jun 23, 2020

Fixes: #3172
Related: n/a
Merge before/after: n/a

Description

Support configuring default namespace for kubectl/kustomize deployers. The Helm deployer luckily already had this (release namespace). In addition, all three deployers now allow templating for those fields, which makes sense e.g. for per-user environments with a namespace of dev-{{.USER}}.

With this change, users don't explicitly have to specify -n targetnamespace all the time, and instead declaratively configure – maybe even per profile – which namespace the application should be deployed to. Command line argument / environment variable still take precedence, as expected in most Unix-like tools.

@AndiDog AndiDog requested a review from a team as a code owner June 23, 2020 14:22
@AndiDog AndiDog requested a review from tstromberg June 23, 2020 14:22
@AndiDog AndiDog force-pushed the namespace-configurable branch from 3f337c9 to 66c1fa8 Compare June 23, 2020 18:57
@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #4374 into master will decrease coverage by 0.01%.
The diff coverage is 65.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4374      +/-   ##
==========================================
- Coverage   73.85%   73.83%   -0.02%     
==========================================
  Files         347      347              
  Lines       13796    13867      +71     
==========================================
+ Hits        10189    10239      +50     
- Misses       2972     2984      +12     
- Partials      635      644       +9     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/deploy/helm.go 76.98% <31.25%> (-1.91%) ⬇️
pkg/skaffold/runner/new.go 66.25% <62.50%> (-1.93%) ⬇️
pkg/skaffold/deploy/kubectl.go 68.26% <77.77%> (+0.13%) ⬆️
pkg/skaffold/deploy/kustomize.go 79.31% <77.77%> (-0.34%) ⬇️
pkg/skaffold/build/cluster/types.go 100.00% <100.00%> (ø)
pkg/skaffold/deploy/kubectl/cli.go 88.63% <100.00%> (ø)
pkg/skaffold/deploy/resource/deployment.go 91.54% <100.00%> (ø)
pkg/skaffold/kubectl/cli.go 100.00% <100.00%> (ø)
pkg/skaffold/sync/types.go 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10275c6...f7e7b94. Read the comment docs.

@gsquared94
Copy link
Contributor

unfortunately the script to generate new config is currently broken. Once that gets fixed and applied you should be able rebase to fix the tests. We'll just have to wait until then, sorry.

@gsquared94
Copy link
Contributor

A new config version was created. Can you try to rebase on master.

@AndiDog AndiDog force-pushed the namespace-configurable branch from 66c1fa8 to ef09cfa Compare July 9, 2020 20:02
@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label Jul 13, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jul 13, 2020
@MarlonGamez
Copy link
Contributor

Hi @AndiDog , it looks like this hasn't been touched for a bit, so I think this PR will need to be rebased. If you fix the conflicts and @ me, I'll try to review this asap

@AndiDog AndiDog force-pushed the namespace-configurable branch from ef09cfa to 4eaf8df Compare September 8, 2020 08:51
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Sep 8, 2020
@AndiDog
Copy link
Contributor Author

AndiDog commented Sep 8, 2020

@MarlonGamez Rebased, and tests + new feature are working for me locally. Didn't run integration tests, so let's see what CI says.

@AndiDog AndiDog force-pushed the namespace-configurable branch from 4eaf8df to f0be4d1 Compare September 8, 2020 10:43
@MarlonGamez
Copy link
Contributor

Hey again, we actually released v2beta7 last week, so I think you'll have to rebase again make the changes to v2beta8. Really sorry that this is turning out to be such a hassle :(

@AndiDog AndiDog force-pushed the namespace-configurable branch from f0be4d1 to f7e7b94 Compare September 17, 2020 04:34
@AndiDog
Copy link
Contributor Author

AndiDog commented Sep 17, 2020

@MarlonGamez Rebased again and recreated the JSON file. Hope it's building now.

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

Just did a final review to double check everything, and it looks good to me :) thanks again for being patient with the rebases and what not

@MarlonGamez MarlonGamez merged commit be196e0 into GoogleContainerTools:master Sep 18, 2020
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.

Add namespace configuration option for kubectl/kustomize
5 participants