Fix issue where default-repo wasn't being added to artifact tags (#5341)

[dan-j]

Fixes: #5341, #5463

Description
Using the --default-repo option on deploy commands wasn't being honoured and the default tag was being deployed.

It's a simple bug... The for-loop in getBuildArtifactsAndSetTags is correctly prefixing the tag with the --default-repo option but was setting it on the for-loop variable (which is a pointer re-used in each loop), and not updating the actual buildArtifacts slice.

I haven't added any tests, this is my first contribution and not sure how to get a runner.Runner in a test environment. If you want tests adding for this change could somebody give me a point in the right direction 🙂

[codecov]

Codecov Report

Merging #5397 (a202f68) into master (4b3c30f) will decrease coverage by 0.10%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #5397      +/-   ##
==========================================
- Coverage   71.60%   71.49%   -0.11%     
==========================================
  Files         397      398       +1     
  Lines       14434    14642     +208     
==========================================
+ Hits        10335    10468     +133     
- Misses       3333     3398      +65     
- Partials      766      776      +10     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	56.52% <0.00%> (ø)
cmd/skaffold/app/cmd/test.go	46.66% <0.00%> (ø)
cmd/skaffold/app/cmd/util.go	77.77% <75.00%> (+12.06%)	⬆️
pkg/skaffold/test/structure/structure.go	87.50% <0.00%> (-12.50%)	⬇️
pkg/skaffold/runner/new.go	67.58% <0.00%> (-1.70%)	⬇️
pkg/skaffold/docker/parse.go	82.16% <0.00%> (-0.81%)	⬇️
pkg/skaffold/util/config.go	77.77% <0.00%> (ø)
pkg/skaffold/errors/errors.go	87.23% <0.00%> (ø)
pkg/skaffold/runner/runner.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/runner.go	60.00% <0.00%> (ø)
... and 25 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 4b3c30f...a202f68. Read the comment docs.

[dan-j]

renamed the variable because it was colliding with the tag package.

[gsquared94]

Thanks for your contribution @dan-j

I haven't added any tests, this is my first contribution and not sure how to get a runner.Runner in a test environment. If you want tests adding for this change could somebody give me a point in the right direction 🙂

You can modify the function signature to:

func getBuildArtifactsAndSetTags(artifacts []*latest.Artifact, applyDefaultRepo func(string)(string, error)) ([]build.Artifact, error) {
  ...

and all non-test references look like:

buildArtifacts, err := getBuildArtifactsAndSetTags(artifacts, r.ApplyDefaultRepo)

That removes the pkg/skaffold/runner dependency from this file.

[dan-j]

Oops, I had forgotten about this and then just ran into the problem this solves 😬

I got halfway through the tests but then sidetracked, will finish this off this week at some point

[MarlonGamez]

hey @dan-j, have you had any time to work on this? Not meaning to pressure, just want to make sure that you have bandwith to finish this :) If not, I'm sure someone on the team could pick it up

[dan-j]

Hey! Yeah I got completely sidetracked by other stuff and forgot, until I've just hit the exact same issue again when running a manual deployment 😅

I was already halfway through the tests, will try finish them tonight, if not it will be this week.

[dan-j]

OK, I've added some tests.

The change @gsquared94 suggested wasn't completely straight forward because that function also uses the global flags for merging build artifacts. I've refactored the problematic code into a new function and have added tests for this new function.

[MarlonGamez]

looks good to me! Thanks @dan-j for this :)