fix: ensure run-id is added to resources in skaffold apply; remove --add-skaffold-labels

Author: nkubala

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

@@ -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

@@ -246,9 +246,7 @@ spec:
 						},
 					},
 				}}),
-				Opts: config.SkaffoldOptions{
-					AddSkaffoldLabels: true,
-				},
+				Opts: config.SkaffoldOptions{},
 			}, &label.DefaultLabeller{}, &latestV1.KubectlDeploy{
 				Manifests: []string{"deployment.yaml"},
 			})

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

@@ -173,7 +173,6 @@ func (rc *RunContext) GetInsecureRegistries() map[string]bool        { return rc
 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

@@ -22,6 +22,7 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy/label"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/event"
@@ -61,7 +62,9 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold
 	isLocalImage := func(imageName string) (bool, error) {
 		return isImageLocal(runCtx, imageName)
 	}
-	labeller := label.NewLabeller(runCtx.AddSkaffoldLabels(), runCtx.CustomLabels(), runCtx.GetRunID())
+
+	// Always add skaffold-specific labels, except during `skaffold render`
+	labeller := label.NewLabeller(runCtx.Mode() != config.RunModes.Render, runCtx.CustomLabels(), runCtx.GetRunID())
 	tester, err := getTester(ctx, runCtx, isLocalImage)
 	if err != nil {
 		endTrace(instrumentation.TraceEndError(err))