feat(helm): add depBuild and template flags to HelmDeployFlags schema by art-shutter · Pull Request #9696 · GoogleContainerTools/skaffold

art-shutter · 2025-01-31T13:24:01Z

Description
This adds new configuration options to HelmDeployFlags struct, DepBuild and Template

User facing changes
Users can now supply new option in the helm config to propagate additional cli flags to helm template and helm dep build commands when using helm as a renderer.

google-cla · 2025-01-31T13:24:06Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ghost

Hi team,

I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, I'm giving you a summary of the pull request's content to help you and other reviewers quickly understand the changes and their intent.

This pull request, authored by art-shutter, aims to add new configuration options to the HelmDeployFlags struct: DepBuild and Template. These additions allow users to supply extra command-line flags to the helm template and helm dep build commands when using Helm as a renderer.

Here's a breakdown of the changes:

docs-v2/content/en/schemas/v4beta12.json: This file has been modified to include the new depBuild and template fields within the HelmDeployFlags schema. Specifically:
- Lines 2300-2311: Added the depBuild field, an array of strings for additional flags to helm dep build.
- Lines 2318-2338: Added the template field, also an array of strings for additional flags to helm template.
- Lines 2331-2334: Updated the preferredOrder array to include depBuild and template.
pkg/skaffold/render/renderer/helm/args.go: This file introduces new functions to handle the additional flags:
- depBuildArgs: Constructs the arguments for helm dep build, including the new flags.
- templateArgs: Constructs the arguments for helm template, incorporating the new flags.
pkg/skaffold/render/renderer/helm/args_test.go: Added comprehensive unit tests for the new functions in args.go.
pkg/skaffold/render/renderer/helm/helm.go: Updated the generateHelmManifest function to use the new depBuildArgs and templateArgs functions and correctly handle the new flags.
pkg/skaffold/schema/latest/config.go: Modified to include the DepBuild and Template fields in the HelmDeployFlags struct (lines 995-1004). Some minor cleanup was also performed on lines 1856-1893.

The changes are well-structured and the unit tests provide good coverage. I'll be looking at the implementation details more closely in my full review.

And now, a little haiku to lighten the mood:

Flags fly in the code,
Helm's commands now more flexible,
Builds run smoother now.

ghost

This pull request adds depBuild and template flags to the HelmDeployFlags schema, allowing users to provide additional CLI flags to helm template and helm dep build commands. The changes are well-implemented and address a user need. The schema changes are clear and the code additions are correct. The tests cover the new functionality appropriately.

Here's a summary of the changes I've suggested, referencing the Google Go Style Guide:

Octal literals: Used octal literals for file permissions (e.g., 0o666 instead of 0666).
Error Wrapping: Wrapped errors using fmt.Errorf for better context and stack traces.

idsulik · 2025-01-31T13:47:33Z

pkg/skaffold/render/renderer/helm/args.go

@@ -0,0 +1,58 @@
+/*
+Copyright 2022 The Skaffold Authors


idsulik · 2025-01-31T13:47:52Z

pkg/skaffold/render/renderer/helm/args_test.go

@@ -0,0 +1,151 @@
+/*
+Copyright 2022 The Skaffold Authors


idsulik · 2025-01-31T14:12:31Z

docs-v2/content/en/schemas/v4beta12.json

          "x-intellij-html-description": "additional flags passed to (<code>helm install</code>).",
          "default": "[]"
        },
+        "template": {


do you need this, or did you add it just in case? if the second one, then it'll be better to remove it, because the more things you add - the more things that should be support, it is better to add as needed

it's added here for consitency. If we don't add this for template then it means the commands helm upgrade and helm install behave differenly from helm template. The latter is used primarily in gitops workflows. Leaving out template would mean that we don't have feature parity when we use skaffold to ask helm to apply the changes directly and when we use helm to template only.

if you need this - keep it) I meant that there is no need to add something if you don't it, because less code - easy to maintain)

idsulik · 2025-01-31T14:19:17Z

pkg/skaffold/render/renderer/helm/helm.go

 		log.Entry(ctx).Info("Building helm dependencies...")
-		if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, "dep", "build", release.ChartPath); err != nil {
+		args := h.depBuildArgs(release.ChartPath)
+		if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, args...); err != nil {


before: it can exit before dep build on lines https://github.com/GoogleContainerTools/skaffold/pull/9696/files#diff-85ea74a2a849b3a19f3a1d48895e2b217f8b1105400c66cae3463582329d07faL165, 171, 175
after: it can exit after dep build, because you moved all the errors inside the templateArgs and call it after dep build.

I'm not sure if it's ok or not, but to preserve the old behavior you can move the templateArgs call before the dep build.

idsulik · 2025-01-31T14:20:32Z

pkg/skaffold/render/renderer/helm/helm.go

@@ -154,17 +154,6 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact,
 		return nil, helm.UserErr(fmt.Sprintf("cannot expand chart path %q", release.ChartPath), err)
 	}



it'll be better to write tests for the generateHelmManifest and refactor it after

yeah, I'd agree. However, the method generateHelmManifest is not atomic. This means the tests would be rather involved, including mocks of charts we pass in. This is exactly why I extracted the functionality that I was adding/modifying to a new method: it lets the tests be written in a saner manner

yes, it was a good idea to extract them and test independently, but at the same time, you've changed the logic as I've explained here #9696 (comment)
let's wait for the mainainers)

…nifest

art-shutter · 2025-02-24T11:41:19Z

@plumpy @menahyouyeah could I get a review from maintainers please? is there a process to request it?

menahyouyeah

Can you rebase? I just merged #9741 which creates a new schema. Then I can merge this PR.

menahyouyeah · 2025-02-27T14:26:47Z

pkg/skaffold/render/renderer/helm/args_test.go

+		additionalArgs []string
+		flags          latest.HelmDeployFlags
+		expected       []string
+		shouldErr      bool


nit: remove - this isnt being used

menahyouyeah · 2025-02-27T14:27:03Z

pkg/skaffold/render/renderer/helm/args_test.go

+				},
+			}
+			args, err := h.templateArgs(test.releaseName, test.release, test.builds, test.namespace, test.additionalArgs)
+			if test.shouldErr {


doesn't look like this is being used - can be removed

…v4beta12

art-shutter · 2025-03-01T10:46:07Z

@menahyouyeah done

menahyouyeah · 2025-03-04T18:12:26Z

Can you fix the failed unit tests? It just looks like the order of the helm render args changed so that's why the tests are failing

fix: reorder the reassigment of args slice to keep the order of the flags

art-shutter · 2025-03-07T11:22:42Z

Can you fix the failed unit tests? It just looks like the order of the helm render args changed so that's why the tests are failing

fixed the order of args in helm commands, since args are positional and expected to be at the positions they were (overrides must always come last)

menahyouyeah · 2025-03-07T14:29:34Z

Can you fix the failed unit tests? It just looks like the order of the helm render args changed so that's why the tests are failing

fixed the order of args in helm commands, since args are positional and expected to be at the positions they were (overrides must always come last)

Just reran the tests and there are some failing - can you take a look?

pkg/skaffold/render/renderer/helm/args.go

tirimia · 2025-03-10T12:01:06Z

pkg/skaffold/render/renderer/helm/args.go

+	return "", nil
+}
+
+func (h Helm) templateArgs(releaseName string, release latest.HelmRelease, builds []graph.Artifact, namespace string, additionalArgs []string) ([]string, error) {


The test suite for this function should include a case where the overrideArgs kick in

…verridesValuesFile function and add tests for Helm template args

tirimia · 2025-03-10T15:29:33Z

pkg/skaffold/render/renderer/helm/args.go

+	defer func() {
+		os.Remove(constants.HelmOverridesFilename)
+	}()


😱 just realized this will now remove the file as the location is returned, this will fail when run

thank you! reverted this back to inlining

art-shutter · 2025-03-11T08:51:21Z

Just reran the tests and there are some failing - can you take a look?

thanks, I did overlook an issue. It is now fixed and locally make unit-tests is not failing for me. Could you please re-run the tests?

menahyouyeah · 2025-03-18T14:01:28Z

Just reran the tests and there are some failing - can you take a look?

thanks, I did overlook an issue. It is now fixed and locally make unit-tests is not failing for me. Could you please re-run the tests?

reran and there are still some broken tests

art-shutter · 2025-03-19T10:58:05Z

reran and there are still some broken tests

from what I can tell based on the logs, this an auth/rate-limiting problem around docker hub, not the code itself
can someone help out and rerun the tests?

vihangm · 2025-04-30T18:34:49Z

Looks like this flag is used by the helm renderer but not the helm deploy.
See https://github.com/GoogleContainerTools/skaffold/blob/a65fe4b/pkg/skaffold/deploy/helm/helm.go#L565-L567 vs https://github.com/GoogleContainerTools/skaffold/blob/759fa38/pkg/skaffold/render/renderer/helm/args.go#L32

However the flag is configurable in both sections of skaffold.yaml, see https://skaffold.dev/docs/references/yaml/#deploy-helm-flags-depBuild

Is this intentional or just an oversight?

art-shutter requested a review from a team as a code owner January 31, 2025 13:24

pull-request-size bot added the size/L label Jan 31, 2025

ghost reviewed Jan 31, 2025

View reviewed changes

feat(helm): add depBuild and template flags to HelmDeployFlags schema

00162ca

art-shutter mentioned this pull request Jan 31, 2025

feat: add buildDependenciesSkipRefresh option to HelmRelease to pass --skip-refresh to helm dep bulid #9672

Closed

fix(renderer): revert file permission mode for HelmOverridesFilename

d2d671b

idsulik reviewed Jan 31, 2025

View reviewed changes

pkg/skaffold/render/renderer/helm/args.go Outdated

@@ -0,0 +1,58 @@

/*

Copyright 2022 The Skaffold Authors

Copy link
Copy Markdown

Contributor

idsulik Jan 31, 2025

Choose a reason for hiding this comment

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

2025

idsulik reviewed Jan 31, 2025

View reviewed changes

fix(helm): move templateArgs call before deps build in generateHelmMa…

7495584

…nifest

This was referenced Feb 26, 2025

new schema version v4beta13 #9740

Closed

chore: new schema version v4beta13 #9741

Merged

menahyouyeah reviewed Feb 27, 2025

View reviewed changes

art-shutter added 3 commits March 1, 2025 11:28

Merge branch 'GoogleContainerTools:main' into add-flags-to-dep-build

d405d59

fix: simplify error handling in TestTemplateArgs

ecf7912

fix: update HelmDeployFlags schema for v4beta13, remove changes from …

14b4a39

…v4beta12

art-shutter added 3 commits March 7, 2025 08:55

Merge branch 'GoogleContainerTools:main' into add-flags-to-dep-build

47561ec

fix: refactor Helm templateArgs to handle overrides and file creation

98dc524

fix: move creation of the overrides values file to a separate function

807273c

fix: reorder the reassigment of args slice to keep the order of the flags

tirimia reviewed Mar 10, 2025

View reviewed changes

fix: remove unneeded append from templateArgs method, improve createO…

c2acaec

…verridesValuesFile function and add tests for Helm template args

tirimia reviewed Mar 10, 2025

View reviewed changes

fix: revert to inline overrides file creation and improve error handling

a0fc64c

menahyouyeah approved these changes Mar 19, 2025

View reviewed changes

menahyouyeah merged commit 759fa38 into GoogleContainerTools:main Mar 19, 2025
11 checks passed

menahyouyeah mentioned this pull request Apr 28, 2025

feat: new schema version with addition to specify contianer build network for buildpack #9745

Open

vihangm mentioned this pull request May 3, 2025

[bug]: Support helm depBuild flags for helm deploys #9808

Merged

menahyouyeah mentioned this pull request Mar 3, 2026

cherrypick #10000

Closed

		@@ -154,17 +154,6 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact,
		return nil, helm.UserErr(fmt.Sprintf("cannot expand chart path %q", release.ChartPath), err)
		}

Conversation

art-shutter commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Jan 31, 2025

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

art-shutter commented Feb 24, 2025

Uh oh!

menahyouyeah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

art-shutter commented Mar 1, 2025

Uh oh!

menahyouyeah commented Mar 4, 2025

Uh oh!

art-shutter commented Mar 7, 2025

Uh oh!

menahyouyeah commented Mar 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

art-shutter commented Mar 11, 2025

Uh oh!

menahyouyeah commented Mar 18, 2025

Uh oh!

art-shutter commented Mar 19, 2025

Uh oh!

Uh oh!

vihangm commented Apr 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

art-shutter commented Jan 31, 2025 •

edited

Loading