Repository: GoogleContainerTools/skaffold
Pull request #8165 (2022-11-28 20:08:39)

docs: remove TODOs from resource selector docs


Patch:
diff --git a/.github/workflows/integration-linux.yml b/.github/workflows/integration-linux.yml
index 93750149c..b3825e6af 100644
--- a/.github/workflows/integration-linux.yml
+++ b/.github/workflows/integration-linux.yml
@@ -19,6 +19,7 @@ jobs:
         kustomize_version: [3.5.4]
         ko_version: [0.4.0]
         kompose_version: [1.21.0]
+        kpt_version: [1.0.0-beta.13]
         gcloud_sdk_version: [410.0.0]
         container_structure_tests_version: [1.8.0]
         integration_test_partitions: [0, 1, 2, 3]
@@ -78,6 +79,12 @@ jobs:
         wget -O kompose https://github.com/kubernetes/kompose/releases/download/v${{ matrix.kompose_version }}/kompose-linux-amd64 && chmod +x kompose
         sudo mv kompose /usr/local/bin/
 
+    - name: Install Kpt
+      if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }}
+      run: |
+        wget -O kpt https://github.com/GoogleContainerTools/kpt/releases/download/v${{ matrix.kpt_version }}/kpt_linux_amd64 && chmod +x kpt
+        sudo mv kpt /usr/local/bin/
+
     - name: Install GCloud
       if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }}
       run: |
diff --git a/docs-v2/content/en/docs/tutorials/skaffold-resource-selector.md b/docs-v2/content/en/docs/tutorials/skaffold-resource-selector.md
index 51757f070..57e423c94 100644
--- a/docs-v2/content/en/docs/tutorials/skaffold-resource-selector.md
+++ b/docs-v2/content/en/docs/tutorials/skaffold-resource-selector.md
@@ -12,7 +12,7 @@ Common Use Cases This Page Helps Resolve:
 * Users who are seeing issues with skaffold's default resource field overwriting for a given resource - eg: skaffold errors as it tries to mutate immutable config on re-deployment
 
 Currently skaffold modifies the manifests it renders and deploys for the following functionality:
-- status checking - done by mutating the manifest/K8s-Object by adding a label - skaffold/dev/run-id.  Skaffold uses this run-id to identify ... (TODO add doc link to run-id explanation)
+- status checking - done by mutating the manifest/K8s-Object by adding a label - skaffold/dev/run-id.  Skaffold uses this run-id to identify the deployments Skaffold manages with it's status checking.
 - image label overwriting - done by mutating the manifest/K8s-Object by substituting the `image:$ORIGINAL_IMAGE_TAG` value(s) in a manifest with `image:$RECENT_SKAFFOLD_BUILT_IMAGE`
 
 
@@ -47,7 +47,7 @@ _* *.image (changes `image:` value to be the skaffold built image ONLY IF skaffo
 
 
 The GroupKind's that Skaffold manages (via resource field overwriting) are user configurable via the `resourceSelector:` top level configuration.  The `resourceSelector` configuration allows users to modify and extend which resources and what fields of those resources skaffold modifies.  Currently skaffold only supports `label:` and `.metadata.labels` related modifications.
-(TODO add `resourceSelector` schema overview and allowable inputs)
+
 `resourceSelector` spec (from `pkg/skaffold/schema/latest/config.go`)
 ```
 // ResourceSelector describes user defined filters describing how skaffold should treat objects/fields during rendering.
diff --git a/pkg/skaffold/deploy/util/util.go b/pkg/skaffold/deploy/util/util.go
index d55ee9db6..b69636154 100644
--- a/pkg/skaffold/deploy/util/util.go
+++ b/pkg/skaffold/deploy/util/util.go
@@ -18,7 +18,6 @@ package util
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -38,7 +37,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util/stringset"
 )
 
@@ -106,10 +104,15 @@ func ConsolidateNamespaces(original, new []string) []string {
 }
 
 // GetHydrationDir points to the directory where the manifest rendering happens. By default, it is set to "<WORKDIR>/.kpt-pipeline".
-func GetHydrationDir(opts config.SkaffoldOptions, workingDir string, promptIfNeeded bool) (string, error) {
+func GetHydrationDir(opts config.SkaffoldOptions, workingDir string, promptIfNeeded bool, isKptRendererOrDeployerUsed bool) (string, error) {
 	var hydratedDir string
 	var err error
 
+	if !isKptRendererOrDeployerUsed {
+		log.Entry(context.TODO()).Info("no kpt renderer or deployer found, skipping hydrated-dir creation")
+		return "", nil
+	}
+
 	if opts.HydrationDir == constants.DefaultHydrationDir {
 		hydratedDir = filepath.Join(workingDir, constants.DefaultHydrationDir)
 		promptIfNeeded = false
@@ -164,37 +167,6 @@ func GroupVersionResource(disco discovery.DiscoveryInterface, gvk schema.GroupVe
 	return false, schema.GroupVersionResource{}, fmt.Errorf("could not find resource for %s", gvk.String())
 }
 
-func GetManifestsFromHydrationDir(ctx context.Context, opts config.SkaffoldOptions) (manifest.ManifestList, error) {
-	workDir, err := util.RealWorkDir()
-	if err != nil {
-		return nil, fmt.Errorf("getting working directory: %w", err)
-	}
-	hydrationDir, err := GetHydrationDir(config.SkaffoldOptions{HydrationDir: opts.HydrationDir, AssumeYes: opts.AssumeYes},
-		workDir,
-		true)
-	if err != nil {
-		return nil, fmt.Errorf("getting render output path: %w", err)
-	}
-	// TODO(aaron-prindle) verify that using manifests.yaml here will work in all cases
-	hydratedManifestPath := filepath.Join(hydrationDir, "manifests.yaml")
-	if _, err := os.Stat(hydratedManifestPath); errors.Is(err, os.ErrNotExist) {
-		log.Entry(ctx).Warn(fmt.Errorf("unable to find manifests, attempted location %s: %w", hydratedManifestPath, err))
-		return nil, nil
-	}
-
-	f, err := os.Open(hydratedManifestPath)
-	if err != nil {
-		return nil, fmt.Errorf("opening hydrated manifest at %s: %w", hydratedManifestPath, err)
-	}
-	defer f.Close()
-
-	manifests, err := manifest.Load(f)
-	if err != nil {
-		return nil, fmt.Errorf("parsing manifests file into manifest list object: %w", err)
-	}
-	return manifests, nil
-}
-
 func GetManifestsFromHydratedManifests(ctx context.Context, hydratedManifests []string) (manifest.ManifestList, error) {
 	var manifests manifest.ManifestList
 	for _, path := range hydratedManifests {
diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go
index 274c50820..1d4d77a50 100644
--- a/pkg/skaffold/runner/new.go
+++ b/pkg/skaffold/runner/new.go
@@ -74,7 +74,7 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold
 
 	var deployer deploy.Deployer
 
-	hydrationDir, err := util.GetHydrationDir(runCtx.Opts, runCtx.WorkingDir, true)
+	hydrationDir, err := util.GetHydrationDir(runCtx.Opts, runCtx.WorkingDir, true, isKptRendererOrDeployerUsed(runCtx.Pipelines))
 
 	if err != nil {
 		return nil, fmt.Errorf("getting render output path: %w", err)
@@ -253,3 +253,21 @@ func getTester(ctx context.Context, cfg test.Config, isLocalImage func(imageName
 
 	return tester, nil
 }
+
+func isKptRendererOrDeployerUsed(pipelines runcontext.Pipelines) bool {
+	for _, configName := range pipelines.AllOrderedConfigNames() {
+		pipeline := pipelines.GetForConfigName(configName)
+		renderConfig := pipeline.Render
+		deployConfig := pipeline.Deploy
+
+		if renderConfig.Validate != nil || renderConfig.Transform != nil || renderConfig.Kpt != nil {
+			return true
+		}
+
+		if deployConfig.KptDeploy != nil {
+			return true
+		}
+	}
+
+	return false
+}


Test patch:
diff --git a/integration/render_test.go b/integration/render_test.go
index 4549f277c..1394bfa07 100644
--- a/integration/render_test.go
+++ b/integration/render_test.go
@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"path/filepath"
 	"regexp"
 	"testing"
 
@@ -705,3 +706,134 @@ spec:
 		})
 	}
 }
+
+func TestRenderHydrationDirCreation(t *testing.T) {
+	MarkIntegrationTest(t, CanRunWithoutGcp)
+	const hydrationDir = "hydration-dir"
+
+	tests := []struct {
+		description              string
+		shouldCreateHydrationDir bool
+		config                   string
+		manifest                 string
+	}{
+		{
+			description:              "project with kpt renderer should create hydration dir",
+			shouldCreateHydrationDir: true,
+			config: `
+apiVersion: skaffold/v4beta1
+kind: Config
+
+build:
+  artifacts: []
+manifests:
+  kpt: []
+`,
+			manifest: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: getting-started
+spec:
+  containers:
+    - name: getting-started
+      image: skaffold-example`,
+		},
+		{
+			description:              "project with kpt deployer should create hydration dir",
+			shouldCreateHydrationDir: true,
+			config: `
+apiVersion: skaffold/v4beta1
+kind: Config
+
+build:
+  artifacts: []
+manifests:
+  kpt: []
+deploy:
+  kpt: {}
+`,
+			manifest: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: getting-started
+spec:
+  containers:
+    - name: getting-started
+      image: skaffold-example`,
+		},
+		{
+			description:              "project with rawYaml and transform should create hydration dir (uses kpt renderer)",
+			shouldCreateHydrationDir: true,
+			config: `
+apiVersion: skaffold/v4beta1
+kind: Config
+
+build:
+  artifacts: []
+manifests:
+  rawYaml:
+    - k8s/k8s-pod.yaml
+  transform:
+    - name: set-annotations
+      configMap:
+        - "author:fake-author"
+`,
+			manifest: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: getting-started
+spec:
+  containers:
+    - name: getting-started
+      image: skaffold-example`,
+		},
+		{
+			description:              "project without kpt should not create hydration dir",
+			shouldCreateHydrationDir: false,
+			config: `
+apiVersion: skaffold/v4beta1
+kind: Config
+
+build:
+  artifacts: []
+manifests:
+  rawYaml:
+    - k8s/k8s-pod.yaml
+`,
+			manifest: `
+apiVersion: v1
+kind: Pod
+metadata:
+  name: getting-started
+spec:
+  containers:
+  - name: getting-started
+    image: skaffold-example`,
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			tmpDir := t.NewTempDir()
+			tmpDir.Mkdir("k8s")
+			tmpDir.Write("skaffold.yaml", test.config)
+			tmpDir.Write("k8s/k8s-pod.yaml", test.manifest)
+			tmpDir.Chdir()
+
+			args := []string{"--hydration-dir", hydrationDir}
+
+			skaffold.Render(args...).RunOrFail(t.T)
+
+			_, err := os.Stat(filepath.Join(tmpDir.Root(), hydrationDir))
+
+			if test.shouldCreateHydrationDir {
+				t.CheckFalse(os.IsNotExist(err))
+			} else {
+				t.CheckTrue(os.IsNotExist(err))
+			}
+		})
+	}
+}
diff --git a/pkg/skaffold/deploy/util/util_test.go b/pkg/skaffold/deploy/util/util_test.go
index 191a66214..f9573b768 100644
--- a/pkg/skaffold/deploy/util/util_test.go
+++ b/pkg/skaffold/deploy/util/util_test.go
@@ -112,7 +112,7 @@ func TestGetHydrationDir_Default(t *testing.T) {
 		tmpDir.Chdir()
 		actual, err := GetHydrationDir(
 			config.SkaffoldOptions{HydrationDir: constants.DefaultHydrationDir, AssumeYes: true},
-			tmpDir.Root(), false)
+			tmpDir.Root(), false, true)
 		t.CheckNoError(err)
 		t.CheckDeepEqual(filepath.Join(tmpDir.Root(), ".kpt-pipeline"), actual)
 	})
@@ -124,7 +124,7 @@ func TestGetHydrationDir_CustomHydrationDir(t *testing.T) {
 		tmpDir.Chdir()
 		expected := filepath.Join(tmpDir.Root(), "test-hydration")
 		actual, err := GetHydrationDir(
-			config.SkaffoldOptions{HydrationDir: expected, AssumeYes: true}, "", false)
+			config.SkaffoldOptions{HydrationDir: expected, AssumeYes: true}, "", false, true)
 		t.CheckNoError(err)
 		t.CheckDeepEqual(expected, actual)
 		_, err = os.Stat(actual)