From aadc1a4de624b7e3973fed9eb9b7359042e993f9 Mon Sep 17 00:00:00 2001 From: Artem Kamenev Date: Mon, 20 Jan 2025 13:05:21 +0100 Subject: [PATCH 1/3] feat: add buildDependenciesSkipRefresh option to HelmRelease for skipping refresh of built dependencies --- docs-v2/content/en/schemas/v4beta12.json | 7 +++++++ pkg/skaffold/render/renderer/helm/helm.go | 6 +++++- pkg/skaffold/schema/latest/config.go | 4 ++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json index 24cd4c39e4d..ac871247196 100755 --- a/docs-v2/content/en/schemas/v4beta12.json +++ b/docs-v2/content/en/schemas/v4beta12.json @@ -2365,6 +2365,12 @@ "name" ], "properties": { + "buildDependenciesSkipRefresh": { + "type": "boolean", + "description": "should the refresh of already built dependencies be skipped. Ignored when `skipBuildDependencies` is `false`.", + "x-intellij-html-description": "should the refresh of already built dependencies be skipped. Ignored when skipBuildDependencies is false.", + "default": "false" + }, "chartPath": { "type": "string", "description": "local path to a packaged Helm chart or an unpacked Helm chart directory.", @@ -2494,6 +2500,7 @@ "wait", "recreatePods", "skipBuildDependencies", + "buildDependenciesSkipRefresh", "skipTests", "useHelmSecrets", "repo", diff --git a/pkg/skaffold/render/renderer/helm/helm.go b/pkg/skaffold/render/renderer/helm/helm.go index b442214e5a6..7e04cdd0ad0 100644 --- a/pkg/skaffold/render/renderer/helm/helm.go +++ b/pkg/skaffold/render/renderer/helm/helm.go @@ -208,7 +208,11 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, // 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 { + cmdArgs := []string{"dep", "build", release.ChartPath} + if release.BuildDependenciesSkipRefresh { + cmdArgs = []string{"dep", "build", "--skip-refresh", release.ChartPath} + } + if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, cmdArgs...); err != nil { log.Entry(ctx).Info(errBuffer.String()) return nil, helm.UserErr("building helm dependencies", err) } diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 90865a21b04..595a904b0bc 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -1050,6 +1050,10 @@ type HelmRelease struct { // Ignored for `remoteChart`. SkipBuildDependencies bool `yaml:"skipBuildDependencies,omitempty"` + // BuildDependenciesSkipRefresh should the refresh of already built dependencies be skipped. + // Ignored when `skipBuildDependencies` is `false`. + BuildDependenciesSkipRefresh bool `yaml:"buildDependenciesSkipRefresh,omitempty"` + // SkipTests should ignore helm test during manifests generation. // Defaults to `false` SkipTests bool `yaml:"skipTests,omitempty"` From e6a93b8247e5187264ac8ac397fa91d329233397 Mon Sep 17 00:00:00 2001 From: Artem Kamenev Date: Tue, 28 Jan 2025 18:16:48 +0100 Subject: [PATCH 2/3] fix: rename buildDependenciesSkipRefresh to skipBuildDependenciesRefresh and update related logic --- docs-v2/content/en/schemas/v4beta12.json | 14 +++--- pkg/skaffold/render/renderer/helm/helm.go | 15 ++++-- .../render/renderer/helm/helm_test.go | 49 +++++++++++++++++++ pkg/skaffold/schema/latest/config.go | 7 ++- 4 files changed, 69 insertions(+), 16 deletions(-) create mode 100644 pkg/skaffold/render/renderer/helm/helm_test.go diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json index ac871247196..6c2485a637e 100755 --- a/docs-v2/content/en/schemas/v4beta12.json +++ b/docs-v2/content/en/schemas/v4beta12.json @@ -2365,12 +2365,6 @@ "name" ], "properties": { - "buildDependenciesSkipRefresh": { - "type": "boolean", - "description": "should the refresh of already built dependencies be skipped. Ignored when `skipBuildDependencies` is `false`.", - "x-intellij-html-description": "should the refresh of already built dependencies be skipped. Ignored when skipBuildDependencies is false.", - "default": "false" - }, "chartPath": { "type": "string", "description": "local path to a packaged Helm chart or an unpacked Helm chart directory.", @@ -2448,6 +2442,12 @@ "x-intellij-html-description": "should build dependencies be skipped. Ignored for remoteChart.", "default": "false" }, + "skipBuildDependenciesRefresh": { + "type": "boolean", + "description": "determines whether the refresh of already built dependencies should be skipped. If set to `true`, passes `--skip-refresh` flag to `helm dep build` command. Ignored when `skipBuildDependencies` is `false`.", + "x-intellij-html-description": "determines whether the refresh of already built dependencies should be skipped. If set to true, passes --skip-refresh flag to helm dep build command. Ignored when skipBuildDependencies is false.", + "default": "false" + }, "skipTests": { "type": "boolean", "description": "should ignore helm test during manifests generation.", @@ -2500,7 +2500,7 @@ "wait", "recreatePods", "skipBuildDependencies", - "buildDependenciesSkipRefresh", + "skipBuildDependenciesRefresh", "skipTests", "useHelmSecrets", "repo", diff --git a/pkg/skaffold/render/renderer/helm/helm.go b/pkg/skaffold/render/renderer/helm/helm.go index 7e04cdd0ad0..76cdd84a6fa 100644 --- a/pkg/skaffold/render/renderer/helm/helm.go +++ b/pkg/skaffold/render/renderer/helm/helm.go @@ -143,6 +143,14 @@ func (h Helm) generateHelmManifests(ctx context.Context, builds []graph.Artifact return manifests, nil } +func buildDepBuildArgs(skipBuildDependenciesRefresh bool) []string { + args := []string{"dep", "build"} + if skipBuildDependenciesRefresh { + args = append(args, "--skip-refresh") + } + return args +} + func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, release latest.HelmRelease, env, additionalArgs []string) ([]byte, error) { releaseName, err := sUtil.ExpandEnvTemplateOrFail(release.Name, nil) if err != nil { @@ -171,7 +179,7 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, return nil, helm.UserErr("cannot marshal overrides to create overrides values.yaml", err) } - if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0666); err != nil { + if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0o666); err != nil { return nil, helm.UserErr(fmt.Sprintf("cannot create file %q", constants.HelmOverridesFilename), err) } @@ -208,10 +216,7 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, // Build Chart dependencies, but allow a user to skip it. if !release.SkipBuildDependencies && release.ChartPath != "" { log.Entry(ctx).Info("Building helm dependencies...") - cmdArgs := []string{"dep", "build", release.ChartPath} - if release.BuildDependenciesSkipRefresh { - cmdArgs = []string{"dep", "build", "--skip-refresh", release.ChartPath} - } + cmdArgs := buildDepBuildArgs(release.SkipBuildDependenciesRefresh) if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, cmdArgs...); err != nil { log.Entry(ctx).Info(errBuffer.String()) return nil, helm.UserErr("building helm dependencies", err) diff --git a/pkg/skaffold/render/renderer/helm/helm_test.go b/pkg/skaffold/render/renderer/helm/helm_test.go new file mode 100644 index 00000000000..f717e87769a --- /dev/null +++ b/pkg/skaffold/render/renderer/helm/helm_test.go @@ -0,0 +1,49 @@ +/* +Copyright 2025 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helm + +import ( + "testing" + + "github.com/GoogleContainerTools/skaffold/v2/testutil" +) + +func TestBuildDepBuildArgs(t *testing.T) { + tests := []struct { + description string + skipBuildDependenciesRefresh bool + expected []string + }{ + { + description: "build args without skipBuildDependenciesRefresh", + skipBuildDependenciesRefresh: false, + expected: []string{"dep", "build"}, + }, + { + description: "build args with skipBuildDependenciesRefresh", + skipBuildDependenciesRefresh: true, + expected: []string{"dep", "build", "--skip-refresh"}, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + args := buildDepBuildArgs(test.skipBuildDependenciesRefresh) + t.CheckDeepEqual(test.expected, args) + }) + } +} diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 595a904b0bc..7f2a58735d9 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -1050,9 +1050,10 @@ type HelmRelease struct { // Ignored for `remoteChart`. SkipBuildDependencies bool `yaml:"skipBuildDependencies,omitempty"` - // BuildDependenciesSkipRefresh should the refresh of already built dependencies be skipped. + // SkipBuildDependenciesRefresh determines whether the refresh of already built dependencies should be skipped. + // If set to `true`, passes `--skip-refresh` flag to `helm dep build` command. // Ignored when `skipBuildDependencies` is `false`. - BuildDependenciesSkipRefresh bool `yaml:"buildDependenciesSkipRefresh,omitempty"` + SkipBuildDependenciesRefresh bool `yaml:"skipBuildDependenciesRefresh,omitempty"` // SkipTests should ignore helm test during manifests generation. // Defaults to `false` @@ -1860,7 +1861,6 @@ func (clusterDetails *ClusterDetails) UnmarshalYAML(value *yaml.Node) error { // Unmarshal the remaining values aux := (*ClusterDetailsForUnmarshaling)(clusterDetails) err = yaml.Unmarshal(remaining, aux) - if err != nil { return err } @@ -1887,7 +1887,6 @@ func (ka *KanikoArtifact) UnmarshalYAML(value *yaml.Node) error { // Unmarshal the remaining values aux := (*KanikoArtifactForUnmarshaling)(ka) err = yaml.Unmarshal(remaining, aux) - if err != nil { return err } From 2f5dc554328357b0e13e3de14a145dbb85ddd0ee Mon Sep 17 00:00:00 2001 From: Artem <19182088+art-shutter@users.noreply.github.com> Date: Thu, 30 Jan 2025 15:52:05 +0100 Subject: [PATCH 3/3] fix: revert gofumpt change --- pkg/skaffold/render/renderer/helm/helm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/render/renderer/helm/helm.go b/pkg/skaffold/render/renderer/helm/helm.go index 76cdd84a6fa..68da6694dba 100644 --- a/pkg/skaffold/render/renderer/helm/helm.go +++ b/pkg/skaffold/render/renderer/helm/helm.go @@ -179,7 +179,7 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, return nil, helm.UserErr("cannot marshal overrides to create overrides values.yaml", err) } - if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0o666); err != nil { + if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0666); err != nil { return nil, helm.UserErr(fmt.Sprintf("cannot create file %q", constants.HelmOverridesFilename), err) }