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

Support templated release names in helm render #5751

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #5671

Description
This PR causes the Helm deployer's Render() needs to expand the release name. It cleans up the unused envs field in the tests.

User facing changes (remove if N/A)

  • render for Helm now supports templated release names

@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #5751 (a57a1ec) into master (4106c53) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5751   +/-   ##
=======================================
  Coverage   70.87%   70.87%           
=======================================
  Files         421      421           
  Lines       16087    16090    +3     
=======================================
+ Hits        11401    11404    +3     
  Misses       3850     3850           
  Partials      836      836           
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm/deploy.go 76.60% <100.00%> (+0.32%) ⬆️

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 4106c53...a57a1ec. Read the comment docs.

@@ -256,9 +256,13 @@ func (h *Deployer) Render(ctx context.Context, out io.Writer, builds []graph.Art
renderedManifests := new(bytes.Buffer)

for _, r := range h.Releases {
args := []string{"template", chartSource(r)}
releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil)
Copy link
Contributor

@aaron-prindle aaron-prindle Apr 29, 2021

Choose a reason for hiding this comment

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

Is releaseName the only field that needs this change?

Wasn't sure if these fields referenced in our-docs/the-bug might need changes as well:
deploy.helm.releases.setValueTemplates
deploy.helm.releases.name
deploy.helm.releases.namespace

If so, perhaps we can add some Github Issues regarding the other required fixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is an excellent question — I looked for other uses of r.Name, but should have checked the others.

  • releases.namespace is expanded in releaseNamespace()
  • setValueTemplates is expanded in constructOverrideArgs()

So I think we're good.

@aaron-prindle
Copy link
Contributor

LGTM

@aaron-prindle
Copy link
Contributor

Side note: TestHelmDeploy is quite large and it is hard to tell exactly what functionality is tested. Would it make sense to split this out somehow? For a new reader of the test it is hard to grasp what is & isn't tested (not sure if there is a simple fix)
https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/deploy/helm/helm_test.go#L470-L1001

@briandealwis briandealwis added the kokoro:force-run forces a kokoro re-run on a PR label Apr 29, 2021
@briandealwis briandealwis enabled auto-merge (squash) April 29, 2021 21:55
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Apr 29, 2021
@briandealwis
Copy link
Member Author

TestHelmDeploy() is a beast! There are many permutations.

@briandealwis briandealwis merged commit 03e9a20 into GoogleContainerTools:master Apr 29, 2021
@briandealwis briandealwis deleted the i5671 branch April 29, 2021 22:19
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.

skaffold render does not respect templating deploy.helm.releases.name
3 participants