Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encourage the use of root_path in production to ensure single deployment #1712

Merged
merged 12 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions bundle/config/mutator/cleanup_targets.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package mutator

import (
"context"
"fmt"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/libs/diag"
)

type cleanupTargets struct {
name string
}

// CleanupTargets cleans up configuration properties before the configuration
// is reported by the 'bundle summary' command.
func CleanupTargets() bundle.Mutator {
return &cleanupTargets{}
}

func (m *cleanupTargets) Name() string {
return fmt.Sprintf("Cleanup(%s)", m.name)
}

func (m *cleanupTargets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
b.Config.Targets = nil
b.Config.Environments = nil
return nil
}
21 changes: 19 additions & 2 deletions bundle/config/mutator/process_target_mode.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,17 @@ 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.
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 specify explicit 'workspace.root_path' to make sure only one copy is deployed")
}
return diag.Errorf("target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed")
}
return nil
}
Expand All @@ -148,6 +157,14 @@ func isRunAsSet(r config.Resources) bool {
return true
}

func isExplicitRootSet(b *bundle.Bundle) bool {
targetConfig := b.Config.Targets[b.Config.Bundle.Target]
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:
Expand Down
22 changes: 20 additions & 2 deletions bundle/config/mutator/process_target_mode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ func mockBundle(mode config.Mode) *bundle.Bundle {
Branch: "main",
},
},
Targets: map[string]*config.Target{
"": {},
},
Workspace: config.Workspace{
CurrentUser: &config.User{
ShortName: "lennart",
Expand Down Expand Up @@ -277,14 +280,14 @@ 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 specify explicit 'workspace.root_path' to make sure only one copy is deployed")

b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state"
b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts"
b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files"

diags = validateProductionMode(context.Background(), b, false)
require.ErrorContains(t, diags.Error(), "production")
require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed")

permissions := []resources.Permission{
{
Expand Down Expand Up @@ -326,6 +329,21 @@ 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.Targets[""].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)
Expand Down
4 changes: 0 additions & 4 deletions bundle/config/mutator/select_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,5 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti
// TODO: remove when Environments section is not supported anymore.
b.Config.Bundle.Environment = b.Config.Bundle.Target

// Clear targets after loading.
b.Config.Targets = nil
b.Config.Environments = nil

return nil
}
3 changes: 2 additions & 1 deletion cmd/bundle/summary.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"path/filepath"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/mutator"
"github.com/databricks/cli/bundle/deploy/terraform"
"github.com/databricks/cli/bundle/phases"
"github.com/databricks/cli/cmd/bundle/utils"
Expand Down Expand Up @@ -60,7 +61,7 @@ func newSummaryCommand() *cobra.Command {
}
}

diags = bundle.Apply(ctx, b, terraform.Load())
diags = bundle.Apply(ctx, b, bundle.Seq(terraform.Load(), mutator.CleanupTargets()))
if err := diags.Error(); err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions cmd/bundle/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/mutator"
"github.com/databricks/cli/bundle/config/validate"
"github.com/databricks/cli/bundle/phases"
"github.com/databricks/cli/bundle/render"
Expand Down Expand Up @@ -65,6 +66,7 @@ func newValidateCommand() *cobra.Command {

return nil
case flags.OutputJSON:
bundle.Apply(ctx, b, mutator.CleanupTargets())
return renderJsonOutput(cmd, b, diags)
default:
return fmt.Errorf("unknown output type %s", root.OutputType(cmd))
Expand Down