Merge remote-tracking branch 'origin' into async-logger-clean

acceptance/acceptance_test.go

@@ -7,6 +7,7 @@ import (
 	"flag"
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -27,6 +28,7 @@ import (
 	"github.com/databricks/cli/libs/testserver"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/iam"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -263,8 +265,23 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
 
 		if len(config.Server) > 0 || config.RecordRequests {
 			server = testserver.New(t)
-			server.RecordRequests = config.RecordRequests
-			server.IncludeRequestHeaders = config.IncludeRequestHeaders
+			if config.RecordRequests {
+				requestsPath := filepath.Join(tmpDir, "out.requests.txt")
+				server.RecordRequestsCallback = func(request *testserver.Request) {
+					req := getLoggedRequest(request, config.IncludeRequestHeaders)
+					reqJson, err := json.MarshalIndent(req, "", "  ")
+					assert.NoErrorf(t, err, "Failed to indent: %#v", req)
+
+					reqJsonWithRepls := repls.Replace(string(reqJson))
+
+					f, err := os.OpenFile(requestsPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o644)
+					assert.NoError(t, err)
+					defer f.Close()
+
+					_, err = f.WriteString(reqJsonWithRepls + "\n")
+					assert.NoError(t, err)
+				}
+			}
 
 			// We want later stubs takes precedence, because then leaf configs take precedence over parent directory configs
 			// In gorilla/mux earlier handlers take precedence, so we need to reverse the order
@@ -345,25 +362,6 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
 	cmd.Dir = tmpDir
 	err = cmd.Run()
 
-	// Write the requests made to the server to a output file if the test is
-	// configured to record requests.
-	if config.RecordRequests {
-		f, err := os.OpenFile(filepath.Join(tmpDir, "out.requests.txt"), os.O_CREATE|os.O_WRONLY, 0o644)
-		require.NoError(t, err)
-
-		for _, req := range server.Requests {
-			reqJson, err := json.MarshalIndent(req, "", "  ")
-			require.NoErrorf(t, err, "Failed to indent: %#v", req)
-
-			reqJsonWithRepls := repls.Replace(string(reqJson))
-			_, err = f.WriteString(reqJsonWithRepls + "\n")
-			require.NoError(t, err)
-		}
-
-		err = f.Close()
-		require.NoError(t, err)
-	}
-
 	// Include exit code in output (if non-zero)
 	formatOutput(out, err)
 	require.NoError(t, out.Close())
@@ -670,3 +668,38 @@ func RunCommand(t *testing.T, args []string, dir string) {
 		t.Logf("%s output: %s", args, out)
 	}
 }
+
+type LoggedRequest struct {
+	Headers http.Header `json:"headers,omitempty"`
+	Method  string      `json:"method"`
+	Path    string      `json:"path"`
+	Body    any         `json:"body,omitempty"`
+	RawBody string      `json:"raw_body,omitempty"`
+}
+
+func getLoggedRequest(req *testserver.Request, includedHeaders []string) LoggedRequest {
+	result := LoggedRequest{
+		Method:  req.Method,
+		Path:    req.URL.Path,
+		Headers: filterHeaders(req.Headers, includedHeaders),
+	}
+
+	if json.Valid(req.Body) {
+		result.Body = json.RawMessage(req.Body)
+	} else {
+		result.RawBody = string(req.Body)
+	}
+
+	return result
+}
+
+func filterHeaders(h http.Header, includedHeaders []string) http.Header {
+	headers := make(http.Header)
+	for k, v := range h {
+		if !slices.Contains(includedHeaders, k) {
+			continue
+		}
+		headers[k] = v
+	}
+	return headers
+}

acceptance/bundle/templates/dbt-sql/output/my_dbt_sql/databricks.yml

@@ -12,12 +12,12 @@ include:
 # The default schema, catalog, etc. for dbt are defined in dbt_profiles/profiles.yml
 targets:
   dev:
-    default: true
     # The default target uses 'mode: development' to create a development copy.
     # - Deployed resources get prefixed with '[dev my_user_name]'
     # - Any job schedules and triggers are paused by default.
     # See also https://docs.databricks.com/dev-tools/bundles/deployment-modes.html.
     mode: development
+    default: true
     workspace:
       host: [DATABRICKS_URL]
 
@@ -25,10 +25,8 @@ targets:
     mode: production
     workspace:
       host: [DATABRICKS_URL]
-      # We explicitly specify /Workspace/Users/[USERNAME] to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/[USERNAME] to make sure we only have a single copy.
       root_path: /Workspace/Users/[USERNAME]/.bundle/${bundle.name}/${bundle.target}
     permissions:
       - user_name: [USERNAME]
         level: CAN_MANAGE
-    run_as:
-      user_name: [USERNAME]

acceptance/bundle/templates/default-python/output/my_default_python/databricks.yml

@@ -22,10 +22,8 @@ targets:
     mode: production
     workspace:
       host: [DATABRICKS_URL]
-      # We explicitly specify /Workspace/Users/[USERNAME] to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/[USERNAME] to make sure we only have a single copy.
       root_path: /Workspace/Users/[USERNAME]/.bundle/${bundle.name}/${bundle.target}
     permissions:
       - user_name: [USERNAME]
         level: CAN_MANAGE
-    run_as:
-      user_name: [USERNAME]

acceptance/bundle/templates/default-sql/output/my_default_sql/databricks.yml

@@ -35,7 +35,7 @@ targets:
     mode: production
     workspace:
       host: [DATABRICKS_URL]
-      # We explicitly specify /Workspace/Users/[USERNAME] to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/[USERNAME] to make sure we only have a single copy.
       root_path: /Workspace/Users/[USERNAME]/.bundle/${bundle.name}/${bundle.target}
     variables:
       warehouse_id: f00dcafe
@@ -44,5 +44,3 @@ targets:
     permissions:
       - user_name: [USERNAME]
         level: CAN_MANAGE
-    run_as:
-      user_name: [USERNAME]

bundle/config/mutator/process_target_mode.go

@@ -134,7 +134,7 @@ func findNonUserPath(b *bundle.Bundle) string {
 	return ""
 }
 
-func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics {
+func validateProductionMode(b *bundle.Bundle, isPrincipalUsed bool) diag.Diagnostics {
 	r := b.Config.Resources
 	for i := range r.Pipelines {
 		if r.Pipelines[i].Development {
@@ -144,8 +144,11 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs
 
 	// 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: /Workspace/Users/%s/.bundle/${bundle.name}/${bundle.target}",
+	advice := "set 'workspace.root_path' to make sure only one copy is deployed"
+	adviceDetail := fmt.Sprintf(
+		"A common practice is to use a username or principal name in this path, i.e. use\n"+
+			"\n"+
+			"  root_path: /Workspace/Users/%s/.bundle/${bundle.name}/${bundle.target}",
 		b.Config.Workspace.CurrentUser.UserName,
 	)
 	if !isExplicitRootSet(b) {
@@ -154,9 +157,21 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs
 			// 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.Recommendationf("target with 'mode: production' should %s", advice)
+			return diag.Diagnostics{
+				{
+					Severity: diag.Recommendation,
+					Summary:  "target with 'mode: production' should " + advice,
+					Detail:   adviceDetail,
+				},
+			}
+		}
+		return diag.Diagnostics{
+			{
+				Severity: diag.Error,
+				Summary:  "target with 'mode: production' must " + advice,
+				Detail:   adviceDetail,
+			},
 		}
-		return diag.Errorf("target with 'mode: production' must %s", advice)
 	}
 	return nil
 }
@@ -188,7 +203,7 @@ func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Di
 		return diags
 	case config.Production:
 		isPrincipal := iamutil.IsServicePrincipal(b.Config.Workspace.CurrentUser.User)
-		return validateProductionMode(ctx, b, isPrincipal)
+		return validateProductionMode(b, isPrincipal)
 	case "":
 		// No action
 	default:

bundle/config/mutator/process_target_mode_test.go

@@ -328,16 +328,16 @@ func TestProcessTargetModeDefault(t *testing.T) {
 func TestProcessTargetModeProduction(t *testing.T) {
 	b := mockBundle(config.Production)
 
-	diags := validateProductionMode(context.Background(), b, false)
-	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: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
+	diags := validateProductionMode(b, false)
+	require.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n  root_path: /Workspace/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"
 	b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files"
 	b.Config.Workspace.ResourcePath = "/Shared/.bundle/x/y/resources"
 
-	diags = validateProductionMode(context.Background(), b, false)
-	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: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
+	diags = validateProductionMode(b, false)
+	require.ErrorContains(t, diags.Error(), "A common practice is to use a username or principal name in this path, i.e. use\n\n  root_path: /Workspace/Users/lennart@company.com/.bundle/${bundle.name}/${bundle.target}")
 
 	permissions := []resources.Permission{
 		{
@@ -357,7 +357,7 @@ func TestProcessTargetModeProduction(t *testing.T) {
 	b.Config.Resources.ModelServingEndpoints["servingendpoint1"].Permissions = permissions
 	b.Config.Resources.Clusters["cluster1"].Permissions = permissions
 
-	diags = validateProductionMode(context.Background(), b, false)
+	diags = validateProductionMode(b, false)
 	require.NoError(t, diags.Error())
 
 	assert.Equal(t, "job1", b.Config.Resources.Jobs["job1"].Name)
@@ -375,11 +375,11 @@ func TestProcessTargetModeProductionOkForPrincipal(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)
+	diags := validateProductionMode(b, false)
 	require.Error(t, diags.Error())
 
 	// ... but we're much less strict when a principal is used
-	diags = validateProductionMode(context.Background(), b, true)
+	diags = validateProductionMode(b, true)
 	require.NoError(t, diags.Error())
 }
 
@@ -387,7 +387,7 @@ 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)
+	diags := validateProductionMode(b, false)
 	require.Error(t, diags.Error())
 
 	// ... but we're okay if we specify a root path
@@ -396,7 +396,7 @@ func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) {
 			RootPath: "some-root-path",
 		},
 	}
-	diags = validateProductionMode(context.Background(), b, false)
+	diags = validateProductionMode(b, false)
 	require.NoError(t, diags.Error())
 }
 

bundle/permissions/permission_diagnostics.go

@@ -9,6 +9,7 @@ import (
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/cli/libs/dyn"
+	"github.com/databricks/cli/libs/iamutil"
 	"github.com/databricks/cli/libs/set"
 )
 
@@ -33,9 +34,25 @@ func (m *permissionDiagnostics) Apply(ctx context.Context, b *bundle.Bundle) dia
 		return nil
 	}
 
+	me := b.Config.Workspace.CurrentUser.User
+	identityType := "user_name"
+	if iamutil.IsServicePrincipal(me) {
+		identityType = "service_principal_name"
+	}
+
 	return diag.Diagnostics{{
-		Severity:  diag.Warning,
-		Summary:   fmt.Sprintf("permissions section should include %s or one of their groups with CAN_MANAGE permissions", b.Config.Workspace.CurrentUser.UserName),
+		Severity: diag.Recommendation,
+		Summary: fmt.Sprintf("permissions section should explicitly include the current deployment identity '%s' or one of its groups\n"+
+			"If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy.\n\n"+
+			"Consider using a adding a top-level permissions section such as the following:\n\n"+
+			"  permissions:\n"+
+			"    - %s: %s\n"+
+			"      level: CAN_MANAGE\n\n"+
+			"See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration.",
+			b.Config.Workspace.CurrentUser.UserName,
+			identityType,
+			b.Config.Workspace.CurrentUser.UserName,
+		),
 		Locations: []dyn.Location{b.Config.GetLocation("permissions")},
 		ID:        diag.PermissionNotIncluded,
 	}}
@@ -46,7 +63,7 @@ func (m *permissionDiagnostics) Apply(ctx context.Context, b *bundle.Bundle) dia
 // target workspace folder.
 //
 // Returns:
-// - isManager: true if the current user is can manage the bundle resources.
+// - canManageBundle: true if the current user or one of their groups can manage the bundle resources.
 // - assistance: advice on who to contact as to manage this project
 func analyzeBundlePermissions(b *bundle.Bundle) (bool, string) {
 	canManageBundle := false

bundle/permissions/permission_diagnostics_test.go

@@ -18,7 +18,14 @@ func TestPermissionDiagnosticsApplySuccess(t *testing.T) {
 		{Level: "CAN_MANAGE", UserName: "testuser@databricks.com"},
 	})
 
-	diags := permissions.PermissionDiagnostics().Apply(context.Background(), b)
+	diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
+	require.NoError(t, diags.Error())
+}
+
+func TestPermissionDiagnosticsEmpty(t *testing.T) {
+	b := mockBundle(nil)
+
+	diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
 	require.NoError(t, diags.Error())
 }
 
@@ -27,9 +34,19 @@ func TestPermissionDiagnosticsApplyFail(t *testing.T) {
 		{Level: "CAN_VIEW", UserName: "testuser@databricks.com"},
 	})
 
-	diags := permissions.PermissionDiagnostics().Apply(context.Background(), b)
-	require.Equal(t, diag.Warning, diags[0].Severity)
-	require.Contains(t, diags[0].Summary, "permissions section should include testuser@databricks.com or one of their groups with CAN_MANAGE permissions")
+	diags := bundle.Apply(context.Background(), b, permissions.PermissionDiagnostics())
+	require.Equal(t, diag.Recommendation, diags[0].Severity)
+
+	expectedMsg := "permissions section should explicitly include the current deployment identity " +
+		"'testuser@databricks.com' or one of its groups\n" +
+		"If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy.\n\n" +
+		"Consider using a adding a top-level permissions section such as the following:\n\n" +
+		"  permissions:\n" +
+		"    - user_name: testuser@databricks.com\n" +
+		"      level: CAN_MANAGE\n\n" +
+		"See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration."
+
+	require.Contains(t, diags[0].Summary, expectedMsg)
 }
 
 func mockBundle(permissions []resources.Permission) *bundle.Bundle {

libs/template/templates/dbt-sql/template/{{.project_name}}/databricks.yml.tmpl

@@ -12,12 +12,12 @@ include:
 # The default schema, catalog, etc. for dbt are defined in dbt_profiles/profiles.yml
 targets:
   dev:
-    default: true
     # The default target uses 'mode: development' to create a development copy.
     # - Deployed resources get prefixed with '[dev my_user_name]'
     # - Any job schedules and triggers are paused by default.
     # See also https://docs.databricks.com/dev-tools/bundles/deployment-modes.html.
     mode: development
+    default: true
     workspace:
       host: {{workspace_host}}
 
@@ -25,10 +25,8 @@ targets:
     mode: production
     workspace:
       host: {{workspace_host}}
-      # We explicitly specify /Workspace/Users/{{user_name}} to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/{{user_name}} to make sure we only have a single copy.
       root_path: /Workspace/Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target}
     permissions:
       - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}
         level: CAN_MANAGE
-    run_as:
-      {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}

libs/template/templates/default-python/template/{{.project_name}}/databricks.yml.tmpl

@@ -22,10 +22,8 @@ targets:
     mode: production
     workspace:
       host: {{workspace_host}}
-      # We explicitly specify /Workspace/Users/{{user_name}} to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/{{user_name}} to make sure we only have a single copy.
       root_path: /Workspace/Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target}
     permissions:
       - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}
         level: CAN_MANAGE
-    run_as:
-      {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}

libs/template/templates/default-sql/template/{{.project_name}}/databricks.yml.tmpl

@@ -42,7 +42,7 @@ targets:
     mode: production
     workspace:
       host: {{workspace_host}}
-      # We explicitly specify /Workspace/Users/{{user_name}} to make sure we only have a single copy.
+      # We explicitly deploy to /Workspace/Users/{{user_name}} to make sure we only have a single copy.
       root_path: /Workspace/Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target}
     variables:
       warehouse_id: {{index ((regexp "[^/]+$").FindStringSubmatch .http_path) 0}}
@@ -51,5 +51,3 @@ targets:
     permissions:
       - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}
         level: CAN_MANAGE
-    run_as:
-      {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}}

libs/testserver/server.go

@@ -7,7 +7,6 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"reflect"
-	"slices"
 	"strings"
 	"sync"
 
@@ -26,18 +25,7 @@ type Server struct {
 	fakeWorkspaces map[string]*FakeWorkspace
 	mu             *sync.Mutex
 
-	RecordRequests        bool
-	IncludeRequestHeaders []string
-
-	Requests []LoggedRequest
-}
-
-type LoggedRequest struct {
-	Headers http.Header `json:"headers,omitempty"`
-	Method  string      `json:"method"`
-	Path    string      `json:"path"`
-	Body    any         `json:"body,omitempty"`
-	RawBody string      `json:"raw_body,omitempty"`
+	RecordRequestsCallback func(request *Request)
 }
 
 type Request struct {
@@ -265,10 +253,9 @@ func (s *Server) Handle(method, path string, handler HandlerFunc) {
 		}
 
 		request := NewRequest(s.t, r, fakeWorkspace)
-		if s.RecordRequests {
-			s.Requests = append(s.Requests, getLoggedRequest(request, s.IncludeRequestHeaders))
+		if s.RecordRequestsCallback != nil {
+			s.RecordRequestsCallback(&request)
 		}
-
 		respAny := handler(request)
 		resp := normalizeResponse(s.t, respAny)
 
@@ -296,33 +283,6 @@ func getToken(r *http.Request) string {
 	return header[len(prefix):]
 }
 
-func getLoggedRequest(req Request, includedHeaders []string) LoggedRequest {
-	result := LoggedRequest{
-		Method:  req.Method,
-		Path:    req.URL.Path,
-		Headers: filterHeaders(req.Headers, includedHeaders),
-	}
-
-	if json.Valid(req.Body) {
-		result.Body = json.RawMessage(req.Body)
-	} else {
-		result.RawBody = string(req.Body)
-	}
-
-	return result
-}
-
-func filterHeaders(h http.Header, includedHeaders []string) http.Header {
-	headers := make(http.Header)
-	for k, v := range h {
-		if !slices.Contains(includedHeaders, k) {
-			continue
-		}
-		headers[k] = v
-	}
-	return headers
-}
-
 func isNil(i any) bool {
 	if i == nil {
 		return true