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 default-repo by supporting nil as default value for flags #5654

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #5588
Related: #5632, #5548

Description
Adds support for "nillable" values like the use of StringOrUndefined used for --default-repo.

Prior to #5632, we never initialized struct values (called Var in pflag). But with the new --port-forward implementation (#5554), we now needed to initialize struct values with possibly different values depending on the command (#5548).

With #5548 we now reset Var type values to their default value, but it turns out that --default-repo had an incorrect default value of the empty string:

Name: "default-repo",
Shorthand: "d",
Usage: "Default repository value (overrides global config)",
Value: &opts.DefaultRepo,
DefValue: "",
FlagAddMethod: "Var",
DefinedOn: []string{"dev", "run", "debug", "deploy", "render", "build", "delete"},

This default value should have been nil: the default repo support differentiates between nil and the empty string "". When nil, Skaffold will look to see if there is a value set in the global config; whereas the empty string indicates that no default-repo should be used.

User facing changes (remove if N/A)

  • the default-repo is able to be used from the global config again

@briandealwis briandealwis requested a review from MarlonGamez April 9, 2021 02:49
@briandealwis briandealwis requested a review from a team as a code owner April 9, 2021 02:49
@google-cla google-cla bot added the cla: yes label Apr 9, 2021
@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #5654 (ec66a62) into master (bcb2eaa) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5654      +/-   ##
==========================================
+ Coverage   70.54%   70.57%   +0.02%     
==========================================
  Files         410      410              
  Lines       15639    15643       +4     
==========================================
+ Hits        11033    11040       +7     
+ Misses       3793     3792       -1     
+ Partials      813      811       -2     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 88.88% <100.00%> (+0.28%) ⬆️
pkg/skaffold/config/types.go 100.00% <100.00%> (ø)
pkg/skaffold/docker/image.go 79.53% <0.00%> (+1.39%) ⬆️

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 bcb2eaa...ec66a62. Read the comment docs.

@tejal29 tejal29 self-requested a review April 12, 2021 20:10
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.

This looks good to me. I've also tested locally to verify it works

@briandealwis briandealwis merged commit d59537c into GoogleContainerTools:master Apr 12, 2021
@briandealwis briandealwis deleted the nillable branch April 12, 2021 20:46
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.

global default-repo is not respected
2 participants