Skip to content

Support --default-repo=‘’ to erase the value from global config#3990

Merged
dgageot merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:fix-3923
Apr 23, 2020
Merged

Support --default-repo=‘’ to erase the value from global config#3990
dgageot merged 1 commit intoGoogleContainerTools:masterfrom
dgageot:fix-3923

Conversation

@dgageot
Copy link
Copy Markdown
Contributor

@dgageot dgageot commented Apr 20, 2020

Fixes #3923

Signed-off-by: David Gageot david@gageot.net

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 20, 2020

Codecov Report

Merging #3990 into master will increase coverage by 0.03%.
The diff coverage is 84.21%.

Impacted Files Coverage Δ
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/config/util.go 70.53% <33.33%> (ø)
pkg/skaffold/config/types.go 90.90% <90.90%> (ø)
cmd/skaffold/app/cmd/flags.go 100.00% <100.00%> (ø)
pkg/skaffold/runner/build_deploy.go 58.10% <100.00%> (ø)
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0.00%> (+2.43%) ⬆️

@dgageot dgageot force-pushed the fix-3923 branch 2 times, most recently from 8d096da to 9367f0d Compare April 21, 2020 17:22
@dgageot dgageot force-pushed the fix-3923 branch 3 times, most recently from dcef125 to 2c20a91 Compare April 23, 2020 07:31
@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Apr 23, 2020

@briandealwis @nkubala I came up with a nicer solution, I think. Feels much less hacky

Copy link
Copy Markdown
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

LGTM: a much nicer solution.

// CLI flag takes precedence.
if cliValue != "" {
return cliValue, nil
if cliValue != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should have a test specifically for GetDefaultRepo.

@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Apr 23, 2020

Let me merge that and add a few more tests. Maybe also change GetLocalCluster to behave the same

@dgageot dgageot merged commit ed51e35 into GoogleContainerTools:master Apr 23, 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.

unable to set an empty default repo

4 participants