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

Add CLI flag `--config` for configuring the global config location #2555

Merged
merged 10 commits into from Aug 14, 2019

Conversation

@corneliusweig
Copy link
Collaborator

corneliusweig commented Jul 26, 2019

So far, skaffold config has a --config flag, to modify skaffold configs with non-standard locations (default is $HOME/.skaffold/config). Other Skaffold commands do not have a flag to set the config location (see #2468).

This PR adds a config flag to configure the location of the global config for the run, dev, debug, build, deploy, delete, and diagnose subcommands. The reason for not making this flag global (such as --color) is that skaffold config has no access to the Skaffold options, so it would require an awkward workaround. Let me know if this makes sense.

To achieve its goal, this PR required a substantial refactoring of the global config handling. I hope it got simpler.

Fixes #2468
Related #2554

@googlebot googlebot added the cla: yes label Jul 26, 2019
@corneliusweig corneliusweig force-pushed the corneliusweig:w/fix-global-config branch 2 times, most recently from 16c1607 to ab53659 Jul 26, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 26, 2019

Codecov Report

Merging #2555 into master will increase coverage by 0.41%.
The diff coverage is 69.02%.

Impacted Files Coverage Δ
pkg/skaffold/config/options.go 100% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/config/unset.go 83.33% <ø> (+67.94%) ⬆️
cmd/skaffold/app/cmd/runner.go 69.23% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/flags.go 100% <ø> (ø) ⬆️
pkg/skaffold/runner/deploy.go 70.58% <0%> (ø) ⬆️
cmd/skaffold/app/cmd/config/list.go 65.21% <100%> (+65.21%) ⬆️
pkg/skaffold/build/local/types.go 68.57% <100%> (ø) ⬆️
pkg/skaffold/runner/runcontext/context.go 62.5% <100%> (ø) ⬆️
pkg/skaffold/config/util.go 63.29% <63.29%> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <76.92%> (-5.07%) ⬇️
... and 6 more
Copy link
Member

nkubala left a comment

this actually looks really good! it's definitely more code than before, but I think in general it's easier to reason about. can you rebase so we can get CI green?

also, is this PR meant to be merged before #2590? i'm a bit confused on the order of them since a lot of the code is the same.

DefaultRepo: "my-public-registry",
},
},
/* todo(corneliusweig): this behavior can be enabled with `mergo.WithAppendSlice` -> clarify requirements

This comment has been minimized.

Copy link
@nkubala

nkubala Aug 8, 2019

Member

is this something you can address right now? what was stopping you from including it before?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Aug 13, 2019

Author Collaborator

The main reason for not including this here is that it is a behavior change. That's why I opened #2554 for clarification and #2590 to implement that behavior on top of this change-set.
I'll remove the commented test from this PR to clarify that distinction in the git history.

Because the `skaffold config` subcommands are in a separate package, they cannot access this flag, therefore this flag cannot be fully global.

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
This package now uses the config utils from pkg/skaffold/config

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
- when retrieving the config for a given kube-context, merge the result with the global values
- cache merged config
- simplify logic of GetInsecureRegistries, GetLocalCluster, and GetDefaultRepo
- allow to override ReadConfigFile with uncached implementation for tests

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig corneliusweig force-pushed the corneliusweig:w/fix-global-config branch from 505f37e to 91a1d0d Aug 13, 2019
@corneliusweig

This comment has been minimized.

Copy link
Collaborator Author

corneliusweig commented Aug 13, 2019

@nkubala Sorry for my slow response. It's rebased now.

PR #2590 depends on this one and should be merged after this one.

@nkubala

This comment has been minimized.

Copy link
Member

nkubala commented Aug 13, 2019

@corneliusweig thanks! I tried rebasing through the github UI, with surprising success....but then I introduced a lint error and wasn't able to fix that through the UI 🤦‍♂ I'll merge this as soon as it's green!

@corneliusweig

This comment has been minimized.

Copy link
Collaborator Author

corneliusweig commented Aug 13, 2019

@nkubala well, that looks like a test flake. Should I trigger again?

Signed-off-by: Cornelius Weig <22861411+corneliusweig@users.noreply.github.com>
@corneliusweig corneliusweig force-pushed the corneliusweig:w/fix-global-config branch from c7ef098 to 2c75a47 Aug 14, 2019
@corneliusweig

This comment has been minimized.

Copy link
Collaborator Author

corneliusweig commented Aug 14, 2019

@nkubala I triggered again, so everything is green now :)

@nkubala nkubala merged commit 8aa4d28 into GoogleContainerTools:master Aug 14, 2019
3 checks passed
3 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro CI build successful.
Details
@corneliusweig corneliusweig deleted the corneliusweig:w/fix-global-config branch Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.