Trim verbose comments to keep only necessary rationale

Author: radakam

bundle/config/workspace.go

@@ -184,10 +184,8 @@ func (w *Workspace) Client(ctx context.Context) (*databricks.WorkspaceClient, er
 
 	switch {
 	case w.Profile != "":
-		// An explicit profile (--profile or workspace.profile) takes precedence
-		// over auth env vars; see databrickscfg.ProfileAuthLoaders (#5096). The
-		// ValidateConfigAndProfileHost check below still enforces that the bundle
-		// host matches the profile host.
+		// An explicit profile wins over auth env vars (#5096).
+		// ValidateConfigAndProfileHost below still checks host agreement.
 		cfg.Loaders = databrickscfg.ProfileAuthLoaders
 	case w.Host != "":
 		// If only the host is configured, we try and unambiguously match it to

bundle/config/workspace_test.go

@@ -156,8 +156,7 @@ func TestWorkspaceClientNormalizesHostBeforeProfileResolution(t *testing.T) {
 }
 
 func TestWorkspaceClientProfileOverridesAuthEnv(t *testing.T) {
-	// An explicit profile (--profile or workspace.profile) must win over auth
-	// env vars, mirroring MustWorkspaceClient (#5096).
+	// An explicit profile must win over auth env vars (#5096).
 	setupWorkspaceTest(t)
 
 	err := databrickscfg.SaveToProfile(t.Context(), &config.Config{
@@ -202,9 +201,8 @@ func TestWorkspaceClientProfileFillsAuthFromEnv(t *testing.T) {
 }
 
 func TestWorkspaceClientHostAndProfileOverridesAuthEnv(t *testing.T) {
-	// A bundle that pins both workspace.host and workspace.profile: the profile
-	// still wins for auth over the env vars, the bundle host is honored, and
-	// ValidateConfigAndProfileHost passes because they agree (#5096).
+	// Bundle pins both workspace.host and workspace.profile: the profile wins
+	// for auth and the host check passes because they agree (#5096).
 	setupWorkspaceTest(t)
 
 	err := databrickscfg.SaveToProfile(t.Context(), &config.Config{

cmd/api/api.go

@@ -98,10 +98,8 @@ func makeCommand(method string) *cobra.Command {
 			}
 
 			if hasProfileFlag {
-				// An explicit --profile takes precedence over auth env vars; see
-				// databrickscfg.ProfileAuthLoaders (#5096). NormalizeDatabricksConfigFromEnv
-				// is skipped too: the host comes from the profile, not DATABRICKS_HOST,
-				// so promoting its ?o=/?a= query params would be wrong.
+				// An explicit --profile wins over auth env vars (#5096); the host
+				// comes from the profile, so skip env host normalization.
 				cfg.Loaders = databrickscfg.ProfileAuthLoaders
 			} else {
 				auth.NormalizeDatabricksConfigFromEnv(cmd.Context(), cfg)

cmd/root/auth.go

@@ -110,17 +110,9 @@ func profileFlagValue(cmd *cobra.Command) (string, bool) {
 	return value, value != ""
 }
 
-// applyProfileAuthPrecedence makes an explicitly selected --profile take
-// precedence over auth environment variables instead of being shadowed by them.
-//
-// With a profile flag we install databrickscfg.ProfileAuthLoaders (the single
-// source of truth for this precedence rule; see its doc and #5096) and skip
-// NormalizeDatabricksConfigFromEnv: the host comes from the profile, not
-// DATABRICKS_HOST, so promoting that env host's ?o=/?a= query params would be
-// wrong. Trade-off: a host-less profile combined with a SPOG-style
-// DATABRICKS_HOST (?o=123) no longer extracts the workspace_id from the query.
-//
-// Without a profile flag we keep the SDK's env-first behavior.
+// applyProfileAuthPrecedence makes an explicit --profile win over auth env vars
+// via ProfileAuthLoaders (#5096), skipping env host normalization since the host
+// comes from the profile. Without a profile flag, env-first behavior is kept.
 func applyProfileAuthPrecedence(ctx context.Context, cfg *config.Config, hasProfileFlag bool) {
 	if hasProfileFlag {
 		cfg.Loaders = databrickscfg.ProfileAuthLoaders

cmd/root/auth_test.go

@@ -454,8 +454,7 @@ token = tst-token
 	require.NoError(t, err)
 
 	t.Setenv("DATABRICKS_CONFIG_FILE", configFile)
-	// direnv-style auth env vars pointing at a different workspace. Before #5096
-	// these shadowed the profile selected with --profile.
+	// Auth env vars for a different workspace; before #5096 these shadowed --profile.
 	t.Setenv("DATABRICKS_HOST", "https://dev.cloud.databricks.test")
 	t.Setenv("DATABRICKS_TOKEN", "dev-token")
 
@@ -522,8 +521,8 @@ token = tst-token
 	require.NoError(t, err)
 
 	t.Setenv("DATABRICKS_CONFIG_FILE", configFile)
-	// Selected via DATABRICKS_CONFIG_PROFILE, not --profile: this keeps the SDK's
-	// env-first precedence, so the auth env vars below must still win (#5096).
+	// Selected via DATABRICKS_CONFIG_PROFILE, not --profile, so env-first
+	// precedence is kept and the auth env vars below still win (#5096).
 	t.Setenv("DATABRICKS_CONFIG_PROFILE", "tst-svc")
 	t.Setenv("DATABRICKS_HOST", "https://dev.cloud.databricks.test")
 	t.Setenv("DATABRICKS_TOKEN", "dev-token")

libs/databrickscfg/loader.go

@@ -14,31 +14,20 @@ import (
 
 var ResolveProfileFromHost = profileFromHostLoader{}
 
-// ResolveNonAuthFromEnv reads config from environment variables, except for the
-// host and any auth credential. It runs before the config file loader when a
-// profile is explicitly selected, so the profile wins over auth env vars: the
-// SDK's default chain reads env before the config file and never overwrites a
-// set field, which would otherwise let env vars shadow the profile (#5096).
+// ResolveNonAuthFromEnv reads non-auth, non-host config from environment
+// variables. See ProfileAuthLoaders for how it fits into the chain.
 var ResolveNonAuthFromEnv = nonAuthEnvLoader{}
 
-// ProfileAuthLoaders is the loader chain to use when a profile is explicitly
-// selected (--profile or a bundle's workspace.profile), and the single source
-// of truth for that precedence rule. The profile must win for host, routing and
-// auth over the matching env vars (DATABRICKS_HOST, DATABRICKS_TOKEN, ...),
-// which the SDK's default env-first chain would otherwise let shadow it (#5096).
+// ProfileAuthLoaders is the loader chain for an explicitly selected profile
+// (--profile or a bundle's workspace.profile). Unlike the SDK's default
+// env-first chain, the profile wins over auth env vars (#5096):
 //
-// This only governs an explicitly selected profile. One picked up from
-// DATABRICKS_CONFIG_PROFILE keeps env-first precedence: reordering two
-// environment signals (DATABRICKS_CONFIG_PROFILE vs DATABRICKS_HOST) is the
-// SDK's domain; we only override when the profile is named out-of-band.
-//
-// Order:
-//  1. ResolveNonAuthFromEnv: non-auth, non-routing env attrs (e.g. cluster_id),
-//     keeping env-wins precedence for those.
+//  1. ResolveNonAuthFromEnv: non-auth env attrs (e.g. cluster_id), env-wins.
 //  2. ConfigFile: the selected profile (host, routing, auth).
-//  3. ConfigAttributes: gap-fills only fields the profile left empty (e.g. a
-//     host-only profile + DATABRICKS_TOKEN, a common CI pattern); it never
-//     overwrites a profile value, so the profile still wins.
+//  3. ConfigAttributes: gap-fills only fields the profile left empty.
+//
+// A profile from DATABRICKS_CONFIG_PROFILE keeps env-first precedence; only an
+// out-of-band profile name triggers this chain.
 var ProfileAuthLoaders = []config.Loader{
 	ResolveNonAuthFromEnv,
 	config.ConfigFile,
@@ -91,25 +80,12 @@ func findMatchingProfile(configFile *config.File, matcher func(*ini.Section) boo
 	return matching[0], nil
 }
 
-// nonAuthEnvSkipAttrs lists SDK config attributes nonAuthEnvLoader must not read
-// from the environment, beyond those caught by HasAuthAttribute. They identify
-// the target workspace/account (host, routing IDs) or steer the auth method but
-// are tagged auth:"-" (collapsed to Internal), so HasAuthAttribute misses them;
-// leaving them to env would let an env var shadow the selected profile (#5096).
-// Skipping only changes precedence: the trailing ConfigAttributes loader still
-// gap-fills any the profile leaves empty.
-//
-//   - host: no `auth` tag at all.
-//   - workspace_id / account_id: routing identifiers; an env var must not route
-//     the profile's credentials elsewhere.
-//   - auth_type: forces a specific auth method.
-//   - discovery_url: redirects OIDC discovery.
-//   - audience: selects the OIDC/workload-identity token audience.
-//   - cloud: steers cloud-specific auth (Azure/GCP/AWS).
-//
-// Non-auth attrs tagged auth:"-" (oauth_callback_port, debug_headers, ...) are
-// intentionally not skipped; TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs
-// guards that every auth-steering internal attribute stays classified.
+// nonAuthEnvSkipAttrs lists env attributes nonAuthEnvLoader must skip on top of
+// those HasAuthAttribute catches: host/routing (host, workspace_id, account_id)
+// and auth-steering fields tagged auth:"-" (auth_type, discovery_url, audience,
+// cloud), which HasAuthAttribute misses. Reading these from env would shadow the
+// selected profile (#5096). TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs
+// guards against SDK drift.
 var nonAuthEnvSkipAttrs = map[string]bool{
 	"host":          true,
 	"workspace_id":  true,
@@ -128,12 +104,11 @@ func (nonAuthEnvLoader) Name() string {
 
 func (nonAuthEnvLoader) Configure(cfg *config.Config) error {
 	for _, attr := range config.ConfigAttributes {
-		// Leave the host and authentication settings for the config file
-		// (i.e. the selected profile) to provide.
+		// Host and auth come from the profile (config file), not env.
 		if nonAuthEnvSkipAttrs[attr.Name] || attr.HasAuthAttribute() {
 			continue
 		}
-		// Match the SDK loader semantics: don't overwrite a value previously set.
+		// Don't overwrite an already-set value (SDK loader semantics).
 		if !attr.IsZero(cfg) {
 			continue
 		}
@@ -144,8 +119,7 @@ func (nonAuthEnvLoader) Configure(cfg *config.Config) error {
 		if err := attr.SetS(cfg, v); err != nil {
 			return err
 		}
-		// Record the source so `databricks auth describe` and debug output
-		// attribute the value to the environment, matching the SDK loader.
+		// Record the source so `auth describe` attributes the value to env.
 		cfg.SetAttrSource(&attr, config.Source{Type: config.SourceEnv, Name: envName})
 	}
 	return nil

libs/databrickscfg/loader_test.go

@@ -66,11 +66,9 @@ func TestResolveNonAuthFromEnvSkipsHostAndAuth(t *testing.T) {
 }
 
 func TestProfileAuthLoadersConflictingEnvAuthMethodErrors(t *testing.T) {
-	// The profile provides a complete PAT, while the env provides a different
-	// complete method (OAuth M2M). The gap-fill loads the empty client_id/secret
-	// from env, so the resolved config carries two auth methods and the SDK
-	// rejects it: the profile wins for the fields it sets, but a conflicting
-	// complete method in env still errors rather than being dropped (#5096).
+	// Profile has a PAT; env gap-fills a different complete method (OAuth M2M).
+	// The config then carries two auth methods, which the SDK rejects rather
+	// than silently dropping the env one (#5096).
 	t.Setenv("DATABRICKS_CLIENT_ID", "env-client-id")
 	t.Setenv("DATABRICKS_CLIENT_SECRET", "env-client-secret")
 
@@ -84,11 +82,9 @@ func TestProfileAuthLoadersConflictingEnvAuthMethodErrors(t *testing.T) {
 	require.ErrorContains(t, err, "more than one authorization method configured")
 }
 
-// TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs guards against an SDK bump
-// silently re-introducing #5096. Every Internal (auth:"-") env-backed attribute
-// that HasAuthAttribute misses must be either skipped (auth-steering) or listed
-// below as a reviewed env-first attribute; a new SDK attribute fails this test
-// until a human classifies it.
+// TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs fails when an SDK bump adds an
+// Internal (auth:"-") env-backed attribute that is neither skipped nor listed as
+// a reviewed env-first attribute, forcing a human to classify it (#5096).
 func TestNonAuthEnvSkipAttrsCoverSDKInternalEnvAttrs(t *testing.T) {
 	knownEnvFirstInternal := map[string]bool{
 		"oauth_callback_port":         true,