fix: ensure run-id is added to resources in skaffold apply

cmd/skaffold/app/cmd/flags.go

@@ -470,19 +470,6 @@ var flagRegistry = []Flag{
 		FlagAddMethod: "IntVar",
 		DefinedOn:     []string{"dev", "debug"},
 	},
-	{
-		Name:     "add-skaffold-labels",
-		Usage:    "Add Skaffold-specific labels to rendered manifest. Custom labels will still be applied. Helpful for GitOps model where Skaffold is not the deployer.",
-		Value:    &opts.AddSkaffoldLabels,
-		DefValue: true,
-		DefValuePerCommand: map[string]interface{}{
-			"render": false,
-		},
-		FlagAddMethod: "BoolVar",
-		DefinedOn:     []string{"dev", "debug", "render", "run"},
-		IsEnum:        true,
-		Deprecated:    "Adding Skaffold-specific labels in `render` is deprecated.",
-	},
 	{
 		Name:          "mute-logs",
 		Usage:         "mute logs for specified stages in pipeline (build, deploy, status-check, none, all)",

integration/apply_test.go

@@ -41,7 +41,7 @@ func TestDiagnoseRenderApply(t *testing.T) {
 
 		tmpDir.Write("skaffold-diagnose.yaml", string(out))
 
-		out = skaffold.Render("--digest-source=local", "--add-skaffold-labels=false", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFailOutput(t.T)
+		out = skaffold.Render("--digest-source=local", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFailOutput(t.T)
 		tmpDir.Write("render.yaml", string(out))
 
 		skaffold.Apply("render.yaml", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFail(t.T)
@@ -66,7 +66,7 @@ func TestRenderApplyHelmDeployment(t *testing.T) {
 
 		tmpDir.Write("skaffold-diagnose.yaml", string(out))
 
-		out = skaffold.Render("--digest-source=local", "--add-skaffold-labels=false", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFailOutput(t.T)
+		out = skaffold.Render("--digest-source=local", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFailOutput(t.T)
 		tmpDir.Write("render.yaml", string(out))
 
 		skaffold.Apply("render.yaml", "-f", "skaffold-diagnose.yaml").InNs(ns.Name).RunOrFail(t.T)
@@ -75,3 +75,12 @@ func TestRenderApplyHelmDeployment(t *testing.T) {
 		t.CheckNotNil(depApp)
 	})
 }
+
+// Ensure that an intentionally broken deployment fails the status check in `skaffold apply`.
+func TestApplyStatusCheckFailure(t *testing.T) {
+	testutil.Run(t, "ApplyStatusCheckFailure", func(t *testutil.T) {
+		err := skaffold.Apply("deployment.yaml").InDir("testdata/apply").Run(t.T)
+
+		t.CheckError(true, err)
+	})
+}

integration/render_test.go

@@ -23,7 +23,6 @@ import (
 	"os"
 	"path"
 	"regexp"
-	"strconv"
 	"testing"
 
 	yaml "gopkg.in/yaml.v2"
@@ -246,9 +245,7 @@ spec:
 						},
 					},
 				}}),
-				Opts: config.SkaffoldOptions{
-					AddSkaffoldLabels: true,
-				},
+				Opts: config.SkaffoldOptions{},
 			}, &label.DefaultLabeller{}, &latestV1.KubectlDeploy{
 				Manifests: []string{"deployment.yaml"},
 			})
@@ -456,7 +453,6 @@ func TestRenderFromBuildOutput(t *testing.T) {
 		config              string
 		buildOutputFilePath string
 		offline             bool
-		addSkaffoldLabels   bool
 		input               map[string]string // file path => content
 		expectedOut         string
 	}{
@@ -477,7 +473,6 @@ deploy:
 `,
 			buildOutputFilePath: "testdata/render/build-output.json",
 			offline:             false,
-			addSkaffoldLabels:   false,
 			input: map[string]string{"deployment.yaml": `
 apiVersion: v1
 kind: Pod
@@ -522,7 +517,6 @@ deploy:
 `,
 			buildOutputFilePath: "testdata/render/build-output.json",
 			offline:             true,
-			addSkaffoldLabels:   false,
 			input: map[string]string{"deployment.yaml": `
 apiVersion: v1
 kind: Pod
@@ -566,7 +560,6 @@ deploy:
 `,
 			buildOutputFilePath: "testdata/render/build-output.json",
 			offline:             true,
-			addSkaffoldLabels:   true,
 			input: map[string]string{"deployment.yaml": `
 apiVersion: v1
 kind: Pod
@@ -583,8 +576,6 @@ spec:
 			expectedOut: `apiVersion: v1
 kind: Pod
 metadata:
-  labels:
-    skaffold.dev/run-id: SOMEDYNAMICVALUE
   name: my-pod-123
 spec:
   containers:
@@ -610,7 +601,6 @@ deploy:
 `,
 			buildOutputFilePath: "testdata/render/build-output.json",
 			offline:             true,
-			addSkaffoldLabels:   false,
 			input: map[string]string{"deployment.yaml": `
 apiVersion: v1
 kind: Pod
@@ -661,7 +651,6 @@ deploy:
 `,
 			buildOutputFilePath: "testdata/render/build-output.json",
 			offline:             true,
-			addSkaffoldLabels:   true,
 			input: map[string]string{"deployment.yaml": `
 apiVersion: v1
 kind: Pod
@@ -686,7 +675,6 @@ resources:
 kind: Pod
 metadata:
   labels:
-    skaffold.dev/run-id: SOMEDYNAMICVALUE
     this-is-from: kustomization.yaml
   name: my-pod-123
 spec:
@@ -715,7 +703,7 @@ spec:
 
 			tmpDir.Chdir()
 
-			args := []string{"--digest-source=local", "--build-artifacts=" + path.Join(testDir, test.buildOutputFilePath), "--add-skaffold-labels=" + strconv.FormatBool(test.addSkaffoldLabels), "--output", "rendered.yaml"}
+			args := []string{"--digest-source=local", "--build-artifacts=" + path.Join(testDir, test.buildOutputFilePath), "--output", "rendered.yaml"}
 
 			if test.offline {
 				env := []string{"KUBECONFIG=not-supposed-to-be-used-in-offline-mode"}

integration/testdata/apply/deployment.yaml

@@ -0,0 +1,17 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: zz-image-doesnt-exist
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: apply-status-check-failure
+  template:
+    metadata:
+      labels:
+        app: apply-status-check-failure
+    spec:
+      containers:
+      - image: zz-image-doesnt-exist
+        name: apply-status-check-failure

integration/testdata/apply/skaffold.yaml

@@ -0,0 +1,2 @@
+apiVersion: skaffold/v2beta24
+kind: Config

pkg/skaffold/config/options.go

@@ -73,31 +73,26 @@ type SkaffoldOptions struct {
 	SkipRender            bool
 	SkipConfigDefaults    bool
 	PropagateProfiles     bool
-	// Add Skaffold-specific labels including runID, deployer labels, etc.
-	// `CustomLabels` are still applied if this is false. Must only be used in
-	// commands which don't deploy (e.g. `skaffold render`) since the runID
-	// label isn't available.
-	AddSkaffoldLabels    bool
-	DetectMinikube       bool
-	IterativeStatusCheck bool
-	ForceLoadImages      bool
-	WaitForConnection    bool
-	MakePathsAbsolute    *bool
-	StatusCheck          BoolOrUndefined
-	PortForward          PortForwardOptions
-	DefaultRepo          StringOrUndefined
-	PushImages           BoolOrUndefined
-	CustomLabels         []string
-	TargetImages         []string
-	Profiles             []string
-	InsecureRegistries   []string
-	ConfigurationFilter  []string
-	HydratedManifests    []string
-	Muted                Muted
-	BuildConcurrency     int
-	WatchPollInterval    int
-	RPCPort              IntOrUndefined
-	RPCHTTPPort          IntOrUndefined
+	DetectMinikube        bool
+	IterativeStatusCheck  bool
+	ForceLoadImages       bool
+	WaitForConnection     bool
+	MakePathsAbsolute     *bool
+	StatusCheck           BoolOrUndefined
+	PortForward           PortForwardOptions
+	DefaultRepo           StringOrUndefined
+	PushImages            BoolOrUndefined
+	CustomLabels          []string
+	TargetImages          []string
+	Profiles              []string
+	InsecureRegistries    []string
+	ConfigurationFilter   []string
+	HydratedManifests     []string
+	Muted                 Muted
+	BuildConcurrency      int
+	WatchPollInterval     int
+	RPCPort               IntOrUndefined
+	RPCHTTPPort           IntOrUndefined
 
 	SyncRemoteCache  SyncRemoteCacheOption
 	WaitForDeletions WaitForDeletions

pkg/skaffold/runner/runcontext/context.go

@@ -166,14 +166,19 @@ func (rc *RunContext) TransformableAllowList() []latestV1.ResourceFilter {
 	return rc.Pipelines.TransformableAllowList()
 }
 
+// AddSkaffoldLabels tells the Runner whether to add skaffold-specific labels.
+// We only ever skip adding labels during a `skaffold render`.
+func (rc *RunContext) AddSkaffoldLabels() bool {
+	return rc.Opts.Mode() != config.RunModes.Render
+}
+
 func (rc *RunContext) DefaultPipeline() latestV1.Pipeline            { return rc.Pipelines.Head() }
 func (rc *RunContext) GetKubeContext() string                        { return rc.KubeContext }
 func (rc *RunContext) GetPipelines() []latestV1.Pipeline             { return rc.Pipelines.All() }
 func (rc *RunContext) GetInsecureRegistries() map[string]bool        { return rc.InsecureRegistries }
 func (rc *RunContext) GetWorkingDir() string                         { return rc.WorkingDir }
 func (rc *RunContext) GetCluster() config.Cluster                    { return rc.Cluster }
 func (rc *RunContext) GetNamespace() string                          { return rc.Opts.Namespace }
-func (rc *RunContext) AddSkaffoldLabels() bool                       { return rc.Opts.AddSkaffoldLabels }
 func (rc *RunContext) AutoBuild() bool                               { return rc.Opts.AutoBuild }
 func (rc *RunContext) AutoDeploy() bool                              { return rc.Opts.AutoDeploy }
 func (rc *RunContext) AutoSync() bool                                { return rc.Opts.AutoSync }

pkg/skaffold/runner/v1/new.go

@@ -61,6 +61,8 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold
 	isLocalImage := func(imageName string) (bool, error) {
 		return isImageLocal(runCtx, imageName)
 	}
+
+	// Always add skaffold-specific labels, except during `skaffold render`
 	labeller := label.NewLabeller(runCtx.AddSkaffoldLabels(), runCtx.CustomLabels(), runCtx.GetRunID())
 	tester, err := getTester(ctx, runCtx, isLocalImage)
 	if err != nil {