fix: remove global helm flags from flags sent to skaffold filter (#…

…9212) * fix: remove global helm flags from flags sent to `skaffold filter` * test: integration and unit tests for helm with global flags
GoogleContainerTools · Dec 11, 2023 · c4af00e · c4af00e
1 parent fb8332e
commit c4af00e
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 2 deletions.
diff --git a/integration/helm_test.go b/integration/helm_test.go
@@ -192,3 +192,12 @@ spec:
 	testutil.CheckError(t, false, err)
 	testutil.CheckDeepEqual(t, expectedOutput, string(fileContent))
 }
+
+func TestHelmDeployWithGlobalFlags(t *testing.T) {
+	MarkIntegrationTest(t, CanRunWithoutGcp)
+	ns, _ := SetupNamespace(t)
+	// To fix #1823, we make use of env variable templating for release name
+	env := []string{fmt.Sprintf("TEST_NS=%s", ns.Name)}
+	skaffold.Deploy("--images", "us-central1-docker.pkg.dev/k8s-skaffold/testing/skaffold-helm", "-p", "helm-with-global-flags").InDir("testdata/helm").InNs(ns.Name).WithEnv(env).RunOrFail(t)
+	skaffold.Delete().InDir("testdata/helm").InNs(ns.Name).WithEnv(env).RunOrFail(t)
+}
diff --git a/integration/render_test.go b/integration/render_test.go
@@ -522,6 +522,85 @@ spec:
       containers:
       - image: skaffold-helm:latest
         name: skaffold-helm
+`,
+		}, {
+			description:      "With Helm global flags",
+			dir:              "testdata/helm-render",
+			args:             []string{"-p", "helm-render-with-global-flags"},
+			withoutBuildJSON: true,
+			expectedOut: `apiVersion: v1
+kind: Service
+metadata:
+  labels:
+    app: skaffold-helm
+    chart: skaffold-helm-0.1.0
+    heritage: Helm
+    release: skaffold-helm
+    skaffold.dev/run-id: phony-run-id
+  name: skaffold-helm-skaffold-helm
+spec:
+  ports:
+  - name: nginx
+    port: 80
+    protocol: TCP
+    targetPort: 80
+  selector:
+    app: skaffold-helm
+    release: skaffold-helm
+  type: ClusterIP
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: skaffold-helm
+    chart: skaffold-helm-0.1.0
+    heritage: Helm
+    release: skaffold-helm
+    skaffold.dev/run-id: phony-run-id
+  name: skaffold-helm
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: skaffold-helm
+      release: skaffold-helm
+  template:
+    metadata:
+      labels:
+        app: skaffold-helm
+        release: skaffold-helm
+        skaffold.dev/run-id: phony-run-id
+    spec:
+      containers:
+      - image: us-central1-docker.pkg.dev/k8s-skaffold/testing/skaffold-helm:sha256-nonsenselettersandnumbers
+        imagePullPolicy: always
+        name: skaffold-helm
+        ports:
+        - containerPort: 80
+        resources: {}
+---
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  annotations: null
+  labels:
+    app: skaffold-helm
+    chart: skaffold-helm-0.1.0
+    heritage: Helm
+    release: skaffold-helm
+  name: skaffold-helm-skaffold-helm
+spec:
+  rules:
+  - http:
+      paths:
+      - backend:
+          service:
+            name: skaffold-helm-skaffold-helm
+            port:
+              number: 80
+        path: /
+        pathType: ImplementationSpecific
 `,
 		},
 	}

diff --git a/integration/testdata/helm-render/skaffold.yaml b/integration/testdata/helm-render/skaffold.yaml
@@ -15,6 +15,17 @@ profiles:
   manifests:
     helm:
       releases:
+      - name: skaffold-helm
+        chartPath: skaffold-helm
+        setValues:
+          pullPolicy: always
+- name: helm-render-with-global-flags
+  manifests:
+    helm:
+      flags:
+        global:
+        - "--debug"
+      releases:
       - name: skaffold-helm
         chartPath: skaffold-helm
         setValues:

diff --git a/integration/testdata/helm/skaffold.yaml b/integration/testdata/helm/skaffold.yaml
@@ -33,4 +33,14 @@ profiles:
           chartPath: skaffold-helm
       flags:
         install:
-          - "--post-renderer=./change_replicas.py"
+          - "--post-renderer=./change_replicas.py"
+- name: helm-with-global-flags
+  deploy:
+    helm:
+      flags:
+        global:
+          - "--debug"
+      releases:
+        # seed test namespace in the release name.
+        - name: skaffold-helm-{{.TEST_NS}}
+          chartPath: skaffold-helm
diff --git a/pkg/skaffold/helm/bin.go b/pkg/skaffold/helm/bin.go
@@ -106,7 +106,6 @@ func generateSkaffoldFilter(h Client, buildsFile string, flags []string) []strin
 	if len(buildsFile) > 0 {
 		args = append(args, "--build-artifacts", buildsFile)
 	}
-	args = append(args, h.GlobalFlags()...)
 
 	if h.KubeConfig() != "" {
 		args = append(args, "--kubeconfig", h.KubeConfig())

diff --git a/pkg/skaffold/helm/bin_test.go b/pkg/skaffold/helm/bin_test.go
@@ -89,6 +89,7 @@ func TestGenerateSkaffoldFilter(t *testing.T) {
 		enableDebug bool
 		buildFile   string
 		result      []string
+		globalFlags []string
 	}{
 		{
 			description: "empty buildfile is skipped",
@@ -105,6 +106,11 @@ func TestGenerateSkaffoldFilter(t *testing.T) {
 			enableDebug: true,
 			result:      []string{"filter", "--kube-context", "kubecontext", "--debugging", "--kubeconfig", "kubeconfig"},
 		},
+		{
+			description: "helm global flags are not included in filter flags",
+			result:      []string{"filter", "--kube-context", "kubecontext", "--kubeconfig", "kubeconfig"},
+			globalFlags: []string{"--debug"},
+		},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
@@ -115,6 +121,7 @@ func TestGenerateSkaffoldFilter(t *testing.T) {
 				kubeContext:        "kubecontext",
 				kubeConfig:         "kubeconfig",
 				manifestsOverrides: map[string]string{},
+				globalFlags:        test.globalFlags,
 			}
 
 			result := generateSkaffoldFilter(h, test.buildFile, []string{})