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

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.

code-review-assist

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.

code-review-assist

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

@@ -2318,6 +2327,15 @@
          "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


 	outBuffer := new(bytes.Buffer)
 	errBuffer := new(bytes.Buffer)

 	// Build Chart dependencies, but allow a user to skip it.
 	if !release.SkipBuildDependencies && release.ChartPath != "" {
 		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 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

code-review-assist bot reviewed Jan 31, 2025

View reviewed changes

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

00162ca

art-shutter force-pushed the add-flags-to-dep-build branch from 0fd151a to 00162ca Compare January 31, 2025 13:28

art-shutter mentioned this pull request Jan 31, 2025

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

Open

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

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

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

@@ -0,0 +1,151 @@

/*

Copyright 2022 The Skaffold Authors

Copy link

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

art-shutter commented Jan 31, 2025 •

edited

Loading

google-cla bot commented Jan 31, 2025

code-review-assist bot left a comment

code-review-assist bot left a comment

idsulik Jan 31, 2025

idsulik Jan 31, 2025

idsulik Jan 31, 2025

art-shutter Jan 31, 2025

idsulik Jan 31, 2025

idsulik Jan 31, 2025

art-shutter Jan 31, 2025

idsulik Jan 31, 2025

art-shutter Jan 31, 2025

idsulik Jan 31, 2025

		@@ -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)
		}

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

Are you sure you want to change the base?

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

Conversation

art-shutter commented Jan 31, 2025 • edited Loading

google-cla bot commented Jan 31, 2025

code-review-assist bot left a comment

Choose a reason for hiding this comment

code-review-assist bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

art-shutter commented Jan 31, 2025 •

edited

Loading