Skip to content

fix: choose cli default-repo over config file #7144

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 3, 2022
Merged

fix: choose cli default-repo over config file #7144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b516c65
fix: choose cli default-repo over config file
tomasmota Feb 27, 2022
7dc6bc4
add unit test
tomasmota Feb 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions pkg/skaffold/config/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,6 @@ func GetCluster(ctx context.Context, opts GetClusterOpts) (Cluster, error) {

var defaultRepo = opts.DefaultRepo

if defaultRepo.Value() != nil && cfg.DefaultRepo != "" {
defaultRepo = NewStringOrUndefined(&cfg.DefaultRepo)
}

if local && defaultRepo.Value() == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

is cfg.DefaultRepo being checked somewhere else? Otherwise I think the author intended this to say:

if defaultRepo.Value() == nil && cfg.DefaultRepo != "" {
		defaultRepo = NewStringOrUndefined(&cfg.DefaultRepo)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't pass in the flag, but have it in my config, it is still being picked up without that piece of code. I would assume that implies that it is checked somewhere else, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

the reason I believe this piece of code is not needed is because typically when we call the following function:

skaffold/pkg/skaffold/deploy/util/util.go

Lines 34 to 52 in 1dbc7a2

// ApplyDefaultRepo applies the default repo to a given image tag.
func ApplyDefaultRepo(globalConfig string, defaultRepo *string, tag string) (string, error) {
repo, err := config.GetDefaultRepo(globalConfig, defaultRepo)
if err != nil {
return "", fmt.Errorf("getting default repo: %w", err)
}
multiLevel, err := config.GetMultiLevelRepo(globalConfig)
if err != nil {
return "", fmt.Errorf("getting multi-level repo support: %w", err)
}
newTag, err := docker.SubstituteDefaultRepoIntoImage(repo, multiLevel, tag)
if err != nil {
return "", fmt.Errorf("applying default repo to %q: %w", tag, err)
}
return newTag, nil
}

We grab the value from the config and compare things then. This is how things usually worked before these few lines were added I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

registry, err := DiscoverLocalRegistry(ctx, kubeContext)
switch {
Expand Down
6 changes: 6 additions & 0 deletions pkg/skaffold/config/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,12 @@ func TestGetCluster(t *testing.T) {
cfg: &ContextConfig{Kubecontext: "not-k3d"},
expected: Cluster{Local: false, LoadImages: false, PushImages: true},
},
{
description: "generic cluster, default repo already defined",
cfg: &ContextConfig{Kubecontext: "anything-else", DefaultRepo: "myrepo"},
defaultRepo: NewStringOrUndefined(&defaultRepo),
expected: Cluster{Local: false, LoadImages: false, PushImages: true, DefaultRepo: NewStringOrUndefined(&defaultRepo)},
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
Expand Down