Merge 6ea53065cf into 2edfb6cfea

Co-authored-by: Pieter Noordhuis <pcnoordhuis@gmail.com>

bundle/config/bundle.go

@@ -18,6 +18,9 @@ type Bundle struct {
 	// Target is set by the mutator that selects the target.
 	Target string `json:"target,omitempty" bundle:"readonly"`
 
+	// TargetConfig stores a snapshot of the target configuration when it was selected by SelectTarget.
+	TargetConfig *Target `json:"target_config,omitempty" bundle:"internal"`
+
 	// DEPRECATED. Left for backward compatibility with Target
 	Environment string `json:"environment,omitempty" bundle:"readonly"`
 

bundle/config/mutator/process_target_mode.go

@@ -2,6 +2,7 @@ package mutator
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	"github.com/databricks/cli/bundle"
@@ -146,8 +147,21 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs
 		}
 	}
 
-	if !isPrincipalUsed && !isRunAsSet(r) {
-		return diag.Errorf("'run_as' must be set for all jobs when using 'mode: production'")
+	// We need to verify that there is only a single deployment of the current target.
+	// The best way to enforce this is to explicitly set root_path.
+	advice := fmt.Sprintf(
+		"set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/%s/.bundle/${bundle.name}/${bundle.target}",
+		b.Config.Workspace.CurrentUser.UserName,
+	)
+	if !isExplicitRootSet(b) {
+		if isRunAsSet(r) || isPrincipalUsed {
+			// Just setting run_as is not enough to guarantee a single deployment,
+			// and neither is setting a principal.
+			// We only show a warning for these cases since we didn't historically
+			// report an error for them.
+			return diag.Warningf("target with 'mode: production' should " + advice)
+		}
+		return diag.Errorf("target with 'mode: production' must " + advice)
 	}
 	return nil
 }
@@ -164,6 +178,17 @@ func isRunAsSet(r config.Resources) bool {
 	return true
 }
 
+func isExplicitRootSet(b *bundle.Bundle) bool {
+	if b.Config.Bundle.TargetConfig == nil {
+		return false
+	}
+	targetConfig := b.Config.Bundle.TargetConfig
+	if targetConfig.Workspace == nil {
+		return false
+	}
+	return targetConfig.Workspace.RootPath != ""
+}
+
 func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
 	switch b.Config.Bundle.Mode {
 	case config.Development:

bundle/config/mutator/process_target_mode_test.go

@@ -307,7 +307,7 @@ func TestProcessTargetModeProduction(t *testing.T) {
 	b := mockBundle(config.Production)
 
 	diags := validateProductionMode(context.Background(), b, false)
-	require.ErrorContains(t, diags.Error(), "run_as")
+	require.ErrorContains(t, diags.Error(), "target with 'mode: production' must set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
 
 	b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state"
 	b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts"
@@ -315,7 +315,7 @@ func TestProcessTargetModeProduction(t *testing.T) {
 	b.Config.Workspace.ResourcePath = "/Shared/.bundle/x/y/resources"
 
 	diags = validateProductionMode(context.Background(), b, false)
-	require.ErrorContains(t, diags.Error(), "production")
+	require.ErrorContains(t, diags.Error(), "target with 'mode: production' must set 'workspace.root_path' to make sure only one copy is deployed. A common practice is to use a username or principal name in this path, i.e. root_path: /Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
 
 	permissions := []resources.Permission{
 		{
@@ -359,6 +359,23 @@ func TestProcessTargetModeProductionOkForPrincipal(t *testing.T) {
 	require.NoError(t, diags.Error())
 }
 
+func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) {
+	b := mockBundle(config.Production)
+
+	// Our target has all kinds of problems when not using service principals ...
+	diags := validateProductionMode(context.Background(), b, false)
+	require.Error(t, diags.Error())
+
+	// ... but we're okay if we specify a root path
+	b.Config.Bundle.TargetConfig = &config.Target{
+		Workspace: &config.Workspace{
+			RootPath: "some-root-path",
+		},
+	}
+	diags = validateProductionMode(context.Background(), b, false)
+	require.NoError(t, diags.Error())
+}
+
 // Make sure that we have test coverage for all resource types
 func TestAllResourcesMocked(t *testing.T) {
 	b := mockBundle(config.Development)

bundle/config/mutator/select_target.go

@@ -15,6 +15,7 @@ type selectTarget struct {
 }
 
 // SelectTarget merges the specified target into the root configuration.
+// After merging, it removes the 'Targets' section from the configuration.
 func SelectTarget(name string) bundle.Mutator {
 	return &selectTarget{
 		name: name,
@@ -31,7 +32,7 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti
 	}
 
 	// Get specified target
-	_, ok := b.Config.Targets[m.name]
+	targetConfig, ok := b.Config.Targets[m.name]
 	if !ok {
 		return diag.Errorf("%s: no such target. Available targets: %s", m.name, strings.Join(maps.Keys(b.Config.Targets), ", "))
 	}
@@ -43,13 +44,15 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti
 	}
 
 	// Store specified target in configuration for reference.
+	b.Config.Bundle.TargetConfig = targetConfig
 	b.Config.Bundle.Target = m.name
 
 	// We do this for backward compatibility.
 	// TODO: remove when Environments section is not supported anymore.
 	b.Config.Bundle.Environment = b.Config.Bundle.Target
 
-	// Clear targets after loading.
+	// Cleanup the original targets and environments sections since they
+	// show up in the JSON output of the 'summary' and 'validate' commands.
 	b.Config.Targets = nil
 	b.Config.Environments = nil
 

bundle/config/root.go

@@ -47,8 +47,8 @@ type Root struct {
 
 	// Targets can be used to differentiate settings and resources between
 	// bundle deployment targets (e.g. development, staging, production).
-	// If not specified, the code below initializes this field with a
-	// single default-initialized target called "default".
+	// Note that this field is set to 'nil' by the SelectTarget mutator;
+	// use Bundle.TargetConfig to access the selected target configuration.
 	Targets map[string]*Target `json:"targets,omitempty"`
 
 	// DEPRECATED. Left for backward compatibility with Targets