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: correct issues with current upgrade logic for artifactOverrides with helm imageStrategy #8066

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Nov 10, 2022

Initially artifactsOverrides and imageStrategy were removed in skaffold v2 as we now use skaffold as a helm --post-renderer. In our upgrade.go for v2beta29 we had some logic added that would migrate people that used those fields to setValues and setValueTemplates. The upgrade.go logic here though did not work properly for multi-config projects as setValueTemplates has unfriendly behaviour when used with multi-config images as noted in #5317. As such this new PR changes upgrade.go to use all setValueTemplates replacements and adds a helm multi-config example as an integration test. This docs PR below might help to explain this a bit as well:
#8093

@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #8066 (a1cf201) into main (290280e) will decrease coverage by 3.98%.
The diff coverage is 53.28%.

❗ Current head a1cf201 differs from pull request most recent head cbf2e74. Consider uploading reports for the commit cbf2e74 to get more accurate results

@@            Coverage Diff             @@
##             main    #8066      +/-   ##
==========================================
- Coverage   70.48%   66.50%   -3.99%     
==========================================
  Files         515      597      +82     
  Lines       23150    29171    +6021     
==========================================
+ Hits        16317    19399    +3082     
- Misses       5776     8332    +2556     
- Partials     1057     1440     +383     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/test.go 44.44% <0.00%> (ø)
cmd/skaffold/app/exitcode.go 100.00% <ø> (+6.66%) ⬆️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/render.go 35.48% <18.18%> (-5.90%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/run.go 64.28% <33.33%> (-9.63%) ⬇️
cmd/skaffold/app/cmd/fix.go 56.41% <37.50%> (-20.07%) ⬇️
... and 394 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 6 times, most recently from 21b2de5 to de108bd Compare November 14, 2022 18:58
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 14, 2022
@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 2 times, most recently from ddfab49 to ab17181 Compare November 14, 2022 19:36
@aaron-prindle aaron-prindle marked this pull request as ready for review November 14, 2022 19:36
@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 6 times, most recently from 3e91bec to dcd7e19 Compare November 14, 2022 20:25
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 14, 2022
@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 3 times, most recently from 162e11f to be474c9 Compare November 14, 2022 22:42
@@ -200,6 +200,12 @@ func (r *imageReplacer) Visit(gk apimachinery.GroupKind, navpath string, o map[s
log.Entry(context.TODO()).Debugf("Couldn't parse image [%s]: %s", image, err.Error())
return false
}
// this is a hack to properly support `imageStrategy=helm+explicitRegistry` from Skaffold v1.X.X
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a bit more detail about this hack? Like with an example of image value, and why we're setting BaseName to Repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that any other regular scenario will conflict with this conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe there is realistically any regular scenario where this will conflict. I have tried with both artifact/artifactOverrides names of the format:

  artifacts:
  - image: gcr.io/aprindle-test-cluster/skaffold-helm
  artifacts:
  - image: skaffold-helm

and this works for both cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment to have more explanation:

	// this is a hack to properly support `imageStrategy=helm+explicitRegistry` from Skaffold v1.X.X which has the form:
	// image: "{{.Values.image.registry}}/{{.Values.image.repository}}:{{.Values.image.tag}}"
	// works by looking for intermediate helm replacement of the form image: <artifactName>/<artifactName>:<artifactName>
	// and treating that as just <artifact> by modifying the parsed representation.

Copy link
Contributor

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

lgtm, but please add some more text about the hack logic.

@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 2 times, most recently from c100fec to 00074ec Compare November 15, 2022 19:40
@aaron-prindle aaron-prindle force-pushed the fix-8008 branch 2 times, most recently from a1cf201 to c211ee7 Compare November 15, 2022 20:43
@aaron-prindle aaron-prindle merged commit 86d0e09 into GoogleContainerTools:main Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants