Skip to content

chore: support artifactOverrides schema functionality from v2beta* in v3alpha* via setValue* #7707

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

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Aug 1, 2022

Fixes #7668

Fixes #6952
Uses the suggestion recommended by @briandealwis in the issue above to migrate artifactOverrides configuration to similar setValues and setValueTemplates configurations.

Prior to this PR, users using artifactOverrides with v2beta* would have their projects broken as artifactOverrides config was dropped going v2beta* -> v3alpha1. This PR parses the v2beta* artifactsOverrides and converts it to appropriate setValues and setValueTemplates config which v3alpha1 supports. There still might be some possible errors with the currently implementation related to the error mentioned in the issue:

This may be a breaking change as some users may have been using image defaults that don't correspond to the image names in the build.artifacts[].image and so their deployments may cease to work.
 We could invoke helm template and then look for matched and unmatched image names. If we found mismatches, Skaffold could warn[or fail with error] the user or abort the upgrade and direct them to do a manual upgrade.

I will make a separate issue tracking the above case which still needs to be accounted for but this way the majority of artifactOverrides users will not be broken in the migration and won't have to manually update their skaffold.yaml

EDIT: Related to above ^^^^ statement. Talked offline w/ Brian, the error referenced above would be an ongoing error and NOT a regression. He is happy with the approach here and the above issue is not high priority (no regression)

NOTE: The changes to examples/templated-fields in this PR were lost when rebasing the v2 branch onto main from #6952 and that is why they are added back here

@aaron-prindle aaron-prindle force-pushed the fix-7668-v2 branch 2 times, most recently from 973e579 to 9191b71 Compare August 1, 2022 03:37
@pull-request-size pull-request-size bot added size/S and removed size/M labels Aug 1, 2022
@aaron-prindle aaron-prindle force-pushed the fix-7668-v2 branch 2 times, most recently from 826724e to b2ee1e2 Compare August 1, 2022 03:45
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #7707 (5d34fbe) into main (290280e) will decrease coverage by 3.93%.
The diff coverage is 53.13%.

@@            Coverage Diff             @@
##             main    #7707      +/-   ##
==========================================
- Coverage   70.48%   66.55%   -3.94%     
==========================================
  Files         515      589      +74     
  Lines       23150    28300    +5150     
==========================================
+ Hits        16317    18834    +2517     
- Misses       5776     8083    +2307     
- Partials     1057     1383     +326     
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/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%) ⬇️
cmd/skaffold/app/cmd/verify.go 41.17% <41.17%> (ø)
... and 337 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@tejal29
Copy link
Contributor

tejal29 commented Aug 1, 2022

Actually, we are getting rid of artifactOverrides in v3 schema. They were confusing and we don't need these with the new helm template post-renderer.
skaffold fix should emit some warning and we can point to doc https://docs.google.com/document/d/1g54UNCK-FYcVz2ZTmx4i3iNUjvWapnnzrtZ4yW8daE8/edit?resourcekey=0-SmqP33WG9HwYUerlr9qJ0A#heading=h.2nx6vufc97a3

@aaron-prindle aaron-prindle requested a review from tejal29 August 1, 2022 17:34
@aaron-prindle
Copy link
Contributor Author

Talked offline, this issue supports the v3 schema removal of artifactOverrides by properly updating v2 schema artifactOverrides users to the correct setValues|setValueTemplates configuration for the v3 schema to do what their artifactOverrides config was doing prior

@aaron-prindle aaron-prindle changed the title chore: support artifactOverrides schema functionality from v2beta* in v3alpha* chore: support artifactOverrides schema functionality from v2beta* in v3alpha* via setValue* Aug 1, 2022
@aaron-prindle aaron-prindle changed the title chore: support artifactOverrides schema functionality from v2beta* in v3alpha* via setValue* chore: support artifactOverrides schema functionality from v2beta* in v3alpha* via setValue* Aug 1, 2022
@tejal29 tejal29 merged commit 3821f00 into GoogleContainerTools:main Aug 1, 2022
@iosifnicolae2
Copy link

setValueTemplates does not work correctly when building multiple skaffold configs - #8008

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Nov 10, 2022

@iosifnicolae2 can you paste a redacted skaffold.yaml showing the config that does not work (the original skaffold/v2beta* skaffold.yaml)? I would love to repro this locally and start working on a fix, currently I am not sure what is missing in our current upgrade.go logic can can't seem to repro this issue locally

@iosifnicolae2
Copy link

@aaron-prindle I've added a GitHub repo and how to reproduce the issue in #8008

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants