Repository: GoogleContainerTools/skaffold
Pull request #7324 (2022-04-19 13:50:31)

fix: change type of BuildpackArtifact.Volumes
Fixes: https://github.com/GoogleContainerTools/skaffold/issues/7020

codecov[bot]: # [Codecov](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) Report
> Merging [#7324](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) (8b2f8bc) into [main](https://codecov.io/gh/GoogleContainerTools/skaffold/commit/290280e8c6c19231e9ed5b5c027f2c01d22f75be?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) (290280e) will **decrease** coverage by `3.01%`.
> The diff coverage is `56.43%`.

```diff
@@            Coverage Diff             @@
##             main    #7324      +/-   ##
==========================================
- Coverage   70.48%   67.47%   -3.02%     
==========================================
  Files         515      557      +42     
  Lines       23150    26582    +3432     
==========================================
+ Hits        16317    17935    +1618     
- Misses       5776     7355    +1579     
- Partials     1057     1292     +235     
```


| [Impacted Files](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) | Coverage Δ | |
|---|---|---|
| [cmd/skaffold/app/cmd/deploy.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZGVwbG95Lmdv) | `52.00% <0.00%> (-1.85%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/filter.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZmlsdGVyLmdv) | `22.22% <0.00%> (-2.78%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/test.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvdGVzdC5nbw==) | `44.44% <0.00%> (ø)` | |
| [cmd/skaffold/skaffold.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL3NrYWZmb2xkLmdv) | `0.00% <0.00%> (ø)` | |
| [cmd/skaffold/app/cmd/inspect\_tests.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvaW5zcGVjdF90ZXN0cy5nbw==) | `62.50% <14.28%> (-1.14%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/render.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvcmVuZGVyLmdv) | `36.66% <14.28%> (-4.72%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/lsp.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvbHNwLmdv) | `28.12% <28.12%> (ø)` | |
| [cmd/skaffold/app/cmd/fix.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZml4Lmdv) | `54.09% <38.88%> (-22.38%)` | :arrow_down: |
| [cmd/skaffold/app/cmd/lint.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvbGludC5nbw==) | `42.85% <42.85%> (ø)` | |
| [cmd/skaffold/app/cmd/find\_configs.go](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#diff-Y21kL3NrYWZmb2xkL2FwcC9jbWQvZmluZF9jb25maWdzLmdv) | `48.88% <46.15%> (+0.24%)` | :arrow_up: |
| ... and [262 more](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7324/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) | |

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools)

tomas-mota: The test failure looks like a flake.

Patch:
diff --git a/cmd/skaffold/app/cmd/filter.go b/cmd/skaffold/app/cmd/filter.go
index 6b0547d87..a13ab4001 100644
--- a/cmd/skaffold/app/cmd/filter.go
+++ b/cmd/skaffold/app/cmd/filter.go
@@ -23,12 +23,14 @@ import (
 	"os"
 
 	"github.com/spf13/cobra"
+	apim "k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/debugging"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
+	rUtil "github.com/GoogleContainerTools/skaffold/pkg/skaffold/render/renderer/util"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
@@ -66,11 +68,14 @@ func runFilter(ctx context.Context, out io.Writer, debuggingFilters bool, buildA
 			return fmt.Errorf("loading manifests: %w", err)
 		}
 
-		manifestList, err = manifestList.SetLabels(pkgutil.EnvSliceToMap(opts.CustomLabels, "="), manifest.NewResourceSelectorLabels(manifest.TransformAllowlist, manifest.TransformDenylist))
+		allow, deny := getTransformList(configs)
+
+		manifestList, err = manifestList.SetLabels(pkgutil.EnvSliceToMap(opts.CustomLabels, "="),
+			manifest.NewResourceSelectorLabels(allow, deny))
 		if err != nil {
 			return err
 		}
-		manifestList, err = manifestList.ReplaceImages(ctx, buildArtifacts, manifest.NewResourceSelectorImages(manifest.TransformAllowlist, manifest.TransformDenylist))
+		manifestList, err = manifestList.ReplaceImages(ctx, buildArtifacts, manifest.NewResourceSelectorImages(allow, deny))
 		if err != nil {
 			return err
 		}
@@ -99,6 +104,36 @@ func runFilter(ctx context.Context, out io.Writer, debuggingFilters bool, buildA
 	})
 }
 
+func getTransformList(configs []util.VersionedConfig) (map[apim.GroupKind]latest.ResourceFilter, map[apim.GroupKind]latest.ResourceFilter) {
+	// TODO: remove code duplication by adding a new Filter method to the runner.
+	// and reuse renderer/util.ConsolidateTransformConfiguration
+
+	allow := manifest.TransformAllowlist
+	deny := manifest.TransformDenylist
+
+	// add default values
+	for _, rf := range manifest.TransformAllowlist {
+		groupKind := apim.ParseGroupKind(rf.GroupKind)
+		allow[groupKind] = rUtil.ConvertJSONPathIndex(rf)
+	}
+	for _, rf := range manifest.TransformDenylist {
+		groupKind := apim.ParseGroupKind(rf.GroupKind)
+		allow[groupKind] = rUtil.ConvertJSONPathIndex(rf)
+	}
+
+	for _, cfg := range configs {
+		for _, rf := range cfg.(*latest.SkaffoldConfig).ResourceSelector.Allow {
+			groupKind := apim.ParseGroupKind(rf.GroupKind)
+			allow[groupKind] = rUtil.ConvertJSONPathIndex(rf)
+		}
+		for _, rf := range cfg.(*latest.SkaffoldConfig).ResourceSelector.Deny {
+			groupKind := apim.ParseGroupKind(rf.GroupKind)
+			deny[groupKind] = rUtil.ConvertJSONPathIndex(rf)
+		}
+	}
+	return allow, deny
+}
+
 func getInsecureRegistries(opts config.SkaffoldOptions, configs []util.VersionedConfig) (map[string]bool, error) {
 	cfgRegistries, err := config.GetInsecureRegistries(opts.GlobalConfig)
 	if err != nil {
diff --git a/pkg/skaffold/build/buildpacks/lifecycle.go b/pkg/skaffold/build/buildpacks/lifecycle.go
index 9e3fe44de..b77c6b4e2 100644
--- a/pkg/skaffold/build/buildpacks/lifecycle.go
+++ b/pkg/skaffold/build/buildpacks/lifecycle.go
@@ -230,20 +230,19 @@ func resolveDependencyImages(artifact *latest.BuildpackArtifact, r ArtifactResol
 
 func containerConfig(artifact *latest.BuildpackArtifact) (pack.ContainerConfig, error) {
 	var vols []string
-	if artifact.Volumes != nil {
-		for _, v := range *artifact.Volumes {
-			if v.Host == "" || v.Target == "" {
-				// in case these slip by the JSON schema
-				return pack.ContainerConfig{}, errors.New("buildpacks volumes must have both host and target")
-			}
-			var spec string
-			if v.Options == "" {
-				spec = fmt.Sprintf("%s:%s", v.Host, v.Target)
-			} else {
-				spec = fmt.Sprintf("%s:%s:%s", v.Host, v.Target, v.Options)
-			}
-			vols = append(vols, spec)
+	for _, v := range artifact.Volumes {
+		if v.Host == "" || v.Target == "" {
+			// in case these slip by the JSON schema
+			return pack.ContainerConfig{}, errors.New("buildpacks volumes must have both host and target")
 		}
+		var spec string
+		if v.Options == "" {
+			spec = fmt.Sprintf("%s:%s", v.Host, v.Target)
+		} else {
+			spec = fmt.Sprintf("%s:%s:%s", v.Host, v.Target, v.Options)
+		}
+		vols = append(vols, spec)
 	}
+
 	return pack.ContainerConfig{Volumes: vols}, nil
 }
diff --git a/pkg/skaffold/render/renderer/util/util.go b/pkg/skaffold/render/renderer/util/util.go
index 7e5cf03f7..0c53ea568 100644
--- a/pkg/skaffold/render/renderer/util/util.go
+++ b/pkg/skaffold/render/renderer/util/util.go
@@ -83,24 +83,24 @@ func ConsolidateTransformConfiguration(cfg render.Config) (map[apim.GroupKind]la
 	// add default values
 	for _, rf := range manifest.TransformAllowlist {
 		groupKind := apim.ParseGroupKind(rf.GroupKind)
-		transformableAllowlist[groupKind] = convertJSONPathIndex(rf)
+		transformableAllowlist[groupKind] = ConvertJSONPathIndex(rf)
 	}
 	for _, rf := range manifest.TransformDenylist {
 		groupKind := apim.ParseGroupKind(rf.GroupKind)
-		transformableDenylist[groupKind] = convertJSONPathIndex(rf)
+		transformableDenylist[groupKind] = ConvertJSONPathIndex(rf)
 	}
 
 	// add user schema values, override defaults
 	for _, rf := range cfg.TransformAllowList() {
 		instrumentation.AddResourceFilter("schema", "allow")
 		groupKind := apim.ParseGroupKind(rf.GroupKind)
-		transformableAllowlist[groupKind] = convertJSONPathIndex(rf)
+		transformableAllowlist[groupKind] = ConvertJSONPathIndex(rf)
 		delete(transformableDenylist, groupKind)
 	}
 	for _, rf := range cfg.TransformDenyList() {
 		instrumentation.AddResourceFilter("schema", "deny")
 		groupKind := apim.ParseGroupKind(rf.GroupKind)
-		transformableDenylist[groupKind] = convertJSONPathIndex(rf)
+		transformableDenylist[groupKind] = ConvertJSONPathIndex(rf)
 		delete(transformableAllowlist, groupKind)
 	}
 
@@ -119,14 +119,14 @@ func ConsolidateTransformConfiguration(cfg render.Config) (map[apim.GroupKind]la
 		for _, rf := range rsc.Allow {
 			instrumentation.AddResourceFilter("cli-flag", "allow")
 			groupKind := apim.ParseGroupKind(rf.GroupKind)
-			transformableAllowlist[groupKind] = convertJSONPathIndex(rf)
+			transformableAllowlist[groupKind] = ConvertJSONPathIndex(rf)
 			delete(transformableDenylist, groupKind)
 		}
 
 		for _, rf := range rsc.Deny {
 			instrumentation.AddResourceFilter("cli-flag", "deny")
 			groupKind := apim.ParseGroupKind(rf.GroupKind)
-			transformableDenylist[groupKind] = convertJSONPathIndex(rf)
+			transformableDenylist[groupKind] = ConvertJSONPathIndex(rf)
 			delete(transformableAllowlist, groupKind)
 		}
 	}
@@ -134,7 +134,7 @@ func ConsolidateTransformConfiguration(cfg render.Config) (map[apim.GroupKind]la
 	return transformableAllowlist, transformableDenylist, nil
 }
 
-func convertJSONPathIndex(rf latest.ResourceFilter) latest.ResourceFilter {
+func ConvertJSONPathIndex(rf latest.ResourceFilter) latest.ResourceFilter {
 	nrf := latest.ResourceFilter{}
 	nrf.GroupKind = rf.GroupKind
 


Test patch:
diff --git a/cmd/skaffold/app/cmd/filter_test.go b/cmd/skaffold/app/cmd/filter_test.go
index 446506a47..eaaf180ce 100644
--- a/cmd/skaffold/app/cmd/filter_test.go
+++ b/cmd/skaffold/app/cmd/filter_test.go
@@ -17,9 +17,17 @@ limitations under the License.
 package cmd
 
 import (
+	"bytes"
+	"context"
+	"io"
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
+	runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext/v2"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -33,3 +41,65 @@ func TestFilterIsHidden(t *testing.T) {
 		t.CheckDeepEqual(true, cmd.Hidden)
 	})
 }
+
+func TestFilterTransform(t *testing.T) {
+	tests := []struct {
+		description    string
+		manifestsStr   string
+		buildArtifacts []graph.Artifact
+		labels         []string
+		expected       string
+	}{
+		{
+			description: "manifests with images",
+			manifestsStr: `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: chartName
+  labels:
+    app: chartName
+spec:
+  replicas: 3
+  template:
+    spec:
+      containers:
+      - name: chartName
+        image: image1`,
+			buildArtifacts: []graph.Artifact{
+				{ImageName: "image1", Tag: "image1:tag1"},
+				{ImageName: "image2", Tag: "image2:tag2"}},
+			labels: []string{"label1=foo", "run.id=random"},
+			expected: `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: chartName
+    label1: foo
+    run.id: random
+  name: chartName
+spec:
+  replicas: 3
+  template:
+    spec:
+      containers:
+      - image: image1:tag1
+        name: chartName`,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&opts, config.SkaffoldOptions{
+				CustomLabels: test.labels,
+			})
+			mockRunner := &mockDevRunner{}
+			t.Override(&createRunner, func(context.Context, io.Writer, config.SkaffoldOptions) (runner.Runner, []util.VersionedConfig, *runcontext.RunContext, error) {
+				return mockRunner, []util.VersionedConfig{&latest.SkaffoldConfig{}}, nil, nil
+			})
+			t.SetStdin([]byte(test.manifestsStr))
+			var b bytes.Buffer
+			err := runFilter(context.TODO(), &b, false, test.buildArtifacts)
+			t.CheckNoError(err)
+			t.CheckDeepEqual(test.expected, b.String())
+		})
+	}
+}
diff --git a/pkg/skaffold/build/buildpacks/lifecycle_test.go b/pkg/skaffold/build/buildpacks/lifecycle_test.go
index cb6b273f7..94f2e1254 100644
--- a/pkg/skaffold/build/buildpacks/lifecycle_test.go
+++ b/pkg/skaffold/build/buildpacks/lifecycle_test.go
@@ -58,23 +58,23 @@ func TestLifecycleStatusCode(t *testing.T) {
 func TestContainerConfig(t *testing.T) {
 	tests := []struct {
 		description string
-		volumes     []latest.BuildpackVolume
+		volumes     []*latest.BuildpackVolume
 		shouldErr   bool
 		expected    pack.ContainerConfig
 	}{
 		{
 			description: "single volume with no options",
-			volumes:     []latest.BuildpackVolume{{Host: "/foo", Target: "/bar"}},
+			volumes:     []*latest.BuildpackVolume{{Host: "/foo", Target: "/bar"}},
 			expected:    pack.ContainerConfig{Volumes: []string{"/foo:/bar"}},
 		},
 		{
 			description: "single volume with  options",
-			volumes:     []latest.BuildpackVolume{{Host: "/foo", Target: "/bar", Options: "rw"}},
+			volumes:     []*latest.BuildpackVolume{{Host: "/foo", Target: "/bar", Options: "rw"}},
 			expected:    pack.ContainerConfig{Volumes: []string{"/foo:/bar:rw"}},
 		},
 		{
 			description: "multiple volumes",
-			volumes: []latest.BuildpackVolume{
+			volumes: []*latest.BuildpackVolume{
 				{Host: "/foo", Target: "/bar", Options: "rw"},
 				{Host: "/bat", Target: "/baz", Options: "ro"},
 			},
@@ -82,12 +82,12 @@ func TestContainerConfig(t *testing.T) {
 		},
 		{
 			description: "missing host is skipped",
-			volumes:     []latest.BuildpackVolume{{Host: "", Target: "/bar"}},
+			volumes:     []*latest.BuildpackVolume{{Host: "", Target: "/bar"}},
 			shouldErr:   true,
 		},
 		{
 			description: "missing target is skipped",
-			volumes:     []latest.BuildpackVolume{{Host: "/foo", Target: ""}},
+			volumes:     []*latest.BuildpackVolume{{Host: "/foo", Target: ""}},
 			shouldErr:   true,
 		},
 	}
@@ -95,7 +95,7 @@ func TestContainerConfig(t *testing.T) {
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			artifact := latest.BuildpackArtifact{
-				Volumes: &test.volumes,
+				Volumes: test.volumes,
 			}
 			result, err := containerConfig(&artifact)
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, result)
diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go
index 6b189fbf3..ae174854c 100644
--- a/pkg/skaffold/schema/latest/config.go
+++ b/pkg/skaffold/schema/latest/config.go
@@ -1057,7 +1057,7 @@ type BuildpackArtifact struct {
 	Dependencies *BuildpackDependencies `yaml:"dependencies,omitempty"`
 
 	// Volumes support mounting host volumes into the container.
-	Volumes *[]BuildpackVolume `yaml:"volumes,omitempty"`
+	Volumes []*BuildpackVolume `yaml:"volumes,omitempty"`
 }
 
 // BuildpackDependencies *alpha* is used to specify dependencies for an artifact built by buildpacks.