diff --git a/pkg/provisioner/backend/backend.go b/pkg/provisioner/backend/backend.go index fb7f42cb2d..0c9483c365 100644 --- a/pkg/provisioner/backend/backend.go +++ b/pkg/provisioner/backend/backend.go @@ -10,13 +10,18 @@ import ( "github.com/cloudposse/atmos/pkg/schema" ) +// ProvisionResult holds the result of a backend provisioning operation. +type ProvisionResult struct { + Warnings []string // Warning messages to display after spinner completes. +} + // BackendCreateFunc is a function that creates a Terraform backend. type BackendCreateFunc func( ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext, -) error +) (*ProvisionResult, error) // BackendDeleteFunc is a function that deletes a Terraform backend. type BackendDeleteFunc func( @@ -190,19 +195,19 @@ func BackendName(backendType string, backendConfig map[string]any) string { } // ProvisionBackend provisions a backend if provisioning is enabled. -// Returns an error if provisioning fails or no provisioner is registered. +// Returns ProvisionResult with any warnings, and an error if provisioning fails or no provisioner is registered. func ProvisionBackend( ctx context.Context, atmosConfig *schema.AtmosConfiguration, componentConfig map[string]any, authContext *schema.AuthContext, -) error { +) (*ProvisionResult, error) { defer perf.Track(atmosConfig, "backend.ProvisionBackend")() // Check if provisioning is enabled. provision, ok := componentConfig["provision"].(map[string]any) if !ok { - return errUtils.Build(errUtils.ErrProvisioningNotConfigured). + return nil, errUtils.Build(errUtils.ErrProvisioningNotConfigured). WithExplanation("No 'provision' configuration found for this component"). WithHint("Add 'provision.backend.enabled: true' to the component's stack configuration"). Err() @@ -210,7 +215,7 @@ func ProvisionBackend( backend, ok := provision["backend"].(map[string]any) if !ok { - return errUtils.Build(errUtils.ErrProvisioningNotConfigured). + return nil, errUtils.Build(errUtils.ErrProvisioningNotConfigured). WithExplanation("No 'provision.backend' configuration found for this component"). WithHint("Add 'provision.backend.enabled: true' to the component's stack configuration"). Err() @@ -218,7 +223,7 @@ func ProvisionBackend( enabled, ok := backend["enabled"].(bool) if !ok || !enabled { - return errUtils.Build(errUtils.ErrProvisioningNotConfigured). + return nil, errUtils.Build(errUtils.ErrProvisioningNotConfigured). WithExplanation("Backend provisioning is not enabled for this component"). WithHint("Set 'provision.backend.enabled: true' in the component's stack configuration"). Err() @@ -227,18 +232,18 @@ func ProvisionBackend( // Get backend configuration. backendConfig, ok := componentConfig["backend"].(map[string]any) if !ok { - return fmt.Errorf("%w: backend configuration not found", errUtils.ErrBackendNotFound) + return nil, fmt.Errorf("%w: backend configuration not found", errUtils.ErrBackendNotFound) } backendType, ok := componentConfig["backend_type"].(string) if !ok { - return fmt.Errorf("%w: backend_type not specified", errUtils.ErrBackendTypeRequired) + return nil, fmt.Errorf("%w: backend_type not specified", errUtils.ErrBackendTypeRequired) } // Get create function for backend type. createFunc := GetBackendCreate(backendType) if createFunc == nil { - return fmt.Errorf("%w: %s", errUtils.ErrCreateNotImplemented, backendType) + return nil, fmt.Errorf("%w: %s", errUtils.ErrCreateNotImplemented, backendType) } // Execute create function. diff --git a/pkg/provisioner/backend/backend_test.go b/pkg/provisioner/backend/backend_test.go index aac5dff575..49ac25b983 100644 --- a/pkg/provisioner/backend/backend_test.go +++ b/pkg/provisioner/backend/backend_test.go @@ -24,8 +24,8 @@ func TestRegisterBackendCreate(t *testing.T) { // Reset registry before test. resetBackendRegistry() - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockProvisioner) @@ -46,12 +46,12 @@ func TestGetBackendCreate_MultipleTypes(t *testing.T) { // Reset registry before test. resetBackendRegistry() - s3Provisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + s3Provisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return &ProvisionResult{}, nil } - gcsProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + gcsProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", s3Provisioner) @@ -109,8 +109,8 @@ func TestGetBackendDelete_MultipleTypes(t *testing.T) { func TestResetRegistryForTesting(t *testing.T) { // Register some functions first. - mockCreator := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + mockCreator := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return &ProvisionResult{}, nil } mockDeleter := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext, force bool) error { return nil @@ -135,8 +135,8 @@ func TestResetRegistryForTesting_ClearsAllEntries(t *testing.T) { // Reset at start. ResetRegistryForTesting() - mockCreator := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + mockCreator := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return &ProvisionResult{}, nil } mockDeleter := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext, force bool) error { return nil @@ -180,7 +180,7 @@ func TestProvisionBackend_NoProvisioningConfiguration(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err, "Should return error when no provisioning configuration exists") assert.ErrorIs(t, err, errUtils.ErrProvisioningNotConfigured) } @@ -203,7 +203,7 @@ func TestProvisionBackend_NoBackendProvisioningConfiguration(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err, "Should return error when no backend provisioning configuration exists") assert.ErrorIs(t, err, errUtils.ErrProvisioningNotConfigured) } @@ -226,7 +226,7 @@ func TestProvisionBackend_ProvisioningDisabled(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err, "Should return error when provisioning is explicitly disabled") assert.ErrorIs(t, err, errUtils.ErrProvisioningNotConfigured) } @@ -247,7 +247,7 @@ func TestProvisionBackend_ProvisioningEnabledMissingField(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err, "Should return error when enabled field is missing") assert.ErrorIs(t, err, errUtils.ErrProvisioningNotConfigured) } @@ -266,7 +266,7 @@ func TestProvisionBackend_MissingBackendConfiguration(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrBackendNotFound) assert.Contains(t, err.Error(), "backend configuration not found") @@ -289,7 +289,7 @@ func TestProvisionBackend_MissingBackendType(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrBackendTypeRequired) assert.Contains(t, err.Error(), "backend_type not specified") @@ -315,7 +315,7 @@ func TestProvisionBackend_UnsupportedBackendType(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrCreateNotImplemented) assert.Contains(t, err.Error(), "unsupported") @@ -332,11 +332,11 @@ func TestProvisionBackend_Success(t *testing.T) { var capturedBackendConfig map[string]any var capturedAuthContext *schema.AuthContext - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { provisionerCalled = true capturedBackendConfig = backendConfig capturedAuthContext = authContext - return nil + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockProvisioner) @@ -354,7 +354,7 @@ func TestProvisionBackend_Success(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.NoError(t, err) assert.True(t, provisionerCalled, "Provisioner should have been called") assert.NotNil(t, capturedBackendConfig) @@ -372,9 +372,9 @@ func TestProvisionBackend_WithAuthContext(t *testing.T) { var capturedAuthContext *schema.AuthContext - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { capturedAuthContext = authContext - return nil + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockProvisioner) @@ -399,7 +399,7 @@ func TestProvisionBackend_WithAuthContext(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, authContext) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, authContext) require.NoError(t, err) require.NotNil(t, capturedAuthContext) require.NotNil(t, capturedAuthContext.AWS) @@ -414,8 +414,8 @@ func TestProvisionBackend_ProvisionerFailure(t *testing.T) { ctx := context.Background() atmosConfig := &schema.AtmosConfiguration{} - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return errors.New("bucket creation failed: permission denied") + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { + return nil, errors.New("bucket creation failed: permission denied") } RegisterBackendCreate("s3", mockProvisioner) @@ -433,7 +433,7 @@ func TestProvisionBackend_ProvisionerFailure(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) require.Error(t, err) assert.Contains(t, err.Error(), "bucket creation failed") assert.Contains(t, err.Error(), "permission denied") @@ -449,14 +449,14 @@ func TestProvisionBackend_MultipleBackendTypes(t *testing.T) { s3Called := false gcsCalled := false - mockS3Provisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockS3Provisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { s3Called = true - return nil + return &ProvisionResult{}, nil } - mockGCSProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockGCSProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { gcsCalled = true - return nil + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockS3Provisioner) @@ -476,7 +476,7 @@ func TestProvisionBackend_MultipleBackendTypes(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfigS3, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfigS3, nil) require.NoError(t, err) assert.True(t, s3Called, "S3 provisioner should have been called") assert.False(t, gcsCalled, "GCS provisioner should not have been called") @@ -499,7 +499,7 @@ func TestProvisionBackend_MultipleBackendTypes(t *testing.T) { }, } - err = ProvisionBackend(ctx, atmosConfig, componentConfigGCS, nil) + _, err = ProvisionBackend(ctx, atmosConfig, componentConfigGCS, nil) require.NoError(t, err) assert.False(t, s3Called, "S3 provisioner should not have been called") assert.True(t, gcsCalled, "GCS provisioner should have been called") @@ -515,11 +515,11 @@ func TestConcurrentBackendProvisioning(t *testing.T) { var callCount int var mu sync.Mutex - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { mu.Lock() callCount++ mu.Unlock() - return nil + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockProvisioner) @@ -550,7 +550,7 @@ func TestConcurrentBackendProvisioning(t *testing.T) { "backend": baseComponentConfig["backend"], "provision": baseComponentConfig["provision"], } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) assert.NoError(t, err) }() } @@ -603,9 +603,9 @@ func TestProvisionBackend_EnabledWrongType(t *testing.T) { resetBackendRegistry() provisionerCalled := false - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*ProvisionResult, error) { provisionerCalled = true - return nil + return &ProvisionResult{}, nil } RegisterBackendCreate("s3", mockProvisioner) @@ -623,7 +623,7 @@ func TestProvisionBackend_EnabledWrongType(t *testing.T) { }, } - err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) + _, err := ProvisionBackend(ctx, atmosConfig, componentConfig, nil) if tt.shouldError { require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrProvisioningNotConfigured) diff --git a/pkg/provisioner/backend/s3.go b/pkg/provisioner/backend/s3.go index b812658980..fb7c125df9 100644 --- a/pkg/provisioner/backend/s3.go +++ b/pkg/provisioner/backend/s3.go @@ -17,7 +17,6 @@ import ( awsIdentity "github.com/cloudposse/atmos/pkg/aws/identity" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/schema" - "github.com/cloudposse/atmos/pkg/ui" ) const ( @@ -144,19 +143,19 @@ func CreateS3Backend( atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext, -) error { +) (*ProvisionResult, error) { defer perf.Track(atmosConfig, "backend.CreateS3Backend")() // Extract and validate required configuration. config, err := extractS3Config(backendConfig) if err != nil { - return err + return nil, err } // Load AWS configuration with auth context. awsConfig, err := loadAWSConfigWithAuth(ctx, config.region, config.roleArn, authContext) if err != nil { - return errUtils.Build(errUtils.ErrLoadAWSConfig). + return nil, errUtils.Build(errUtils.ErrLoadAWSConfig). WithHint("Check AWS credentials are configured correctly"). WithHintf("Verify AWS region '%s' is valid", config.region). WithHint("If using --identity flag, ensure the identity is authenticated"). @@ -171,16 +170,18 @@ func CreateS3Backend( // Check if bucket exists and create if needed. bucketAlreadyExisted, err := ensureBucket(ctx, client, config.bucket, config.region) if err != nil { - return err + return nil, err } // Apply hardcoded defaults. - // If bucket already existed, warn that settings may be overwritten. - if err := applyS3BucketDefaults(ctx, client, config.bucket, bucketAlreadyExisted); err != nil { - return fmt.Errorf(errFormat, errUtils.ErrApplyBucketDefaults, err) + // If bucket already existed, warnings are returned (not printed directly) to avoid + // concurrent output issues when running inside a spinner. + warnings, err := applyS3BucketDefaults(ctx, client, config.bucket, bucketAlreadyExisted) + if err != nil { + return nil, fmt.Errorf(errFormat, errUtils.ErrApplyBucketDefaults, err) } - return nil + return &ProvisionResult{Warnings: warnings}, nil } // extractS3Config extracts and validates required S3 configuration. @@ -367,12 +368,15 @@ func createBucket(ctx context.Context, client S3ClientAPI, bucket, region string // - Public Access: BLOCKED (all 4 settings) // - Tags: Replaces entire tag set with Name and ManagedBy=Atmos only // -// If the bucket already existed (alreadyExisted=true), warnings are logged to inform the user -// that existing settings are being modified. -func applyS3BucketDefaults(ctx context.Context, client S3ClientAPI, bucket string, alreadyExisted bool) error { - // Warn user if modifying pre-existing bucket settings. +// If the bucket already existed (alreadyExisted=true), warnings are returned to inform the user +// that existing settings are being modified. Warnings are returned instead of printed directly +// to avoid concurrent output issues when called inside a spinner. +func applyS3BucketDefaults(ctx context.Context, client S3ClientAPI, bucket string, alreadyExisted bool) ([]string, error) { + var warnings []string + + // Collect warning if modifying pre-existing bucket settings. if alreadyExisted { - _ = ui.Warning(fmt.Sprintf("Applying Atmos defaults to existing bucket `%s`\n\n"+ + warnings = append(warnings, fmt.Sprintf("Applying Atmos defaults to existing bucket `%s`\n\n"+ "- Versioning will be ENABLED\n"+ "- Encryption will be set to AES-256 (existing KMS encryption will be replaced)\n"+ "- Public access will be BLOCKED (all 4 settings)\n"+ @@ -381,33 +385,33 @@ func applyS3BucketDefaults(ctx context.Context, client S3ClientAPI, bucket strin // 1. Enable versioning (ALWAYS). if err := enableVersioning(ctx, client, bucket); err != nil { - return fmt.Errorf(errFormat, errUtils.ErrEnableVersioning, err) + return nil, fmt.Errorf(errFormat, errUtils.ErrEnableVersioning, err) } // Skip operations not supported by gofakes3 during testing. // These are tested separately with mocks in s3_test.go. if getS3SkipUnsupportedTestOps() { - return nil + return warnings, nil } // 2. Enable encryption with AES-256 (ALWAYS). // NOTE: This replaces any existing encryption configuration, including KMS. if err := enableEncryption(ctx, client, bucket); err != nil { - return fmt.Errorf(errFormat, errUtils.ErrEnableEncryption, err) + return nil, fmt.Errorf(errFormat, errUtils.ErrEnableEncryption, err) } // 3. Block public access (ALWAYS). if err := blockPublicAccess(ctx, client, bucket); err != nil { - return fmt.Errorf(errFormat, errUtils.ErrBlockPublicAccess, err) + return nil, fmt.Errorf(errFormat, errUtils.ErrBlockPublicAccess, err) } // 4. Apply standard tags (ALWAYS). // NOTE: This replaces the entire tag set. Existing tags are not preserved. if err := applyTags(ctx, client, bucket); err != nil { - return fmt.Errorf(errFormat, errUtils.ErrApplyTags, err) + return nil, fmt.Errorf(errFormat, errUtils.ErrApplyTags, err) } - return nil + return warnings, nil } // enableVersioning enables versioning on an S3 bucket. diff --git a/pkg/provisioner/backend/s3_e2e_test.go b/pkg/provisioner/backend/s3_e2e_test.go index 739d6b9409..6150588e4d 100644 --- a/pkg/provisioner/backend/s3_e2e_test.go +++ b/pkg/provisioner/backend/s3_e2e_test.go @@ -84,7 +84,7 @@ func TestE2E_CreateS3Backend_WithMockAWSProvider(t *testing.T) { "bucket": "e2e-test-bucket", "region": "us-east-1", } - err = CreateS3Backend(ctx, nil, backendConfig, authContext) + _, err = CreateS3Backend(ctx, nil, backendConfig, authContext) require.NoError(t, err, "CreateS3Backend should succeed") // Verify bucket was created. @@ -138,7 +138,7 @@ func TestE2E_DeleteS3Backend_WithMockAWSProvider(t *testing.T) { "bucket": bucketName, "region": "us-east-1", } - err = CreateS3Backend(ctx, nil, backendConfig, authContext) + _, err = CreateS3Backend(ctx, nil, backendConfig, authContext) require.NoError(t, err, "CreateS3Backend should succeed") // Verify bucket exists. diff --git a/pkg/provisioner/backend/s3_test.go b/pkg/provisioner/backend/s3_test.go index bc461c1ae0..bdcb1c0e12 100644 --- a/pkg/provisioner/backend/s3_test.go +++ b/pkg/provisioner/backend/s3_test.go @@ -705,8 +705,9 @@ func TestApplyS3BucketDefaults_NewBucket(t *testing.T) { }, } - err := applyS3BucketDefaults(ctx, mockClient, "new-bucket", false) + warnings, err := applyS3BucketDefaults(ctx, mockClient, "new-bucket", false) require.NoError(t, err) + assert.Empty(t, warnings, "No warnings should be returned for new bucket") assert.True(t, versioningCalled, "Versioning should be enabled") assert.True(t, encryptionCalled, "Encryption should be enabled") assert.True(t, publicAccessCalled, "Public access should be blocked") @@ -737,9 +738,11 @@ func TestApplyS3BucketDefaults_ExistingBucket(t *testing.T) { } // With alreadyExisted=true, all operations should still be called. - err := applyS3BucketDefaults(ctx, mockClient, "existing-bucket", true) + warnings, err := applyS3BucketDefaults(ctx, mockClient, "existing-bucket", true) require.NoError(t, err) assert.Equal(t, 4, callCount, "All 4 operations should be called") + assert.Len(t, warnings, 1, "Warning should be returned for existing bucket") + assert.Contains(t, warnings[0], "Applying Atmos defaults to existing bucket") } func TestApplyS3BucketDefaults_VersioningFails(t *testing.T) { @@ -750,7 +753,7 @@ func TestApplyS3BucketDefaults_VersioningFails(t *testing.T) { }, } - err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) + _, err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrEnableVersioning) } @@ -766,7 +769,7 @@ func TestApplyS3BucketDefaults_EncryptionFails(t *testing.T) { }, } - err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) + _, err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrEnableEncryption) } @@ -785,7 +788,7 @@ func TestApplyS3BucketDefaults_PublicAccessFails(t *testing.T) { }, } - err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) + _, err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrBlockPublicAccess) } @@ -807,7 +810,7 @@ func TestApplyS3BucketDefaults_TaggingFails(t *testing.T) { }, } - err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) + _, err := applyS3BucketDefaults(ctx, mockClient, "test-bucket", false) require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrApplyTags) } diff --git a/pkg/provisioner/backend_hook.go b/pkg/provisioner/backend_hook.go index 383c56d1f8..02db19815d 100644 --- a/pkg/provisioner/backend_hook.go +++ b/pkg/provisioner/backend_hook.go @@ -9,6 +9,7 @@ import ( "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/provisioner/backend" "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/ui" "github.com/cloudposse/atmos/pkg/ui/spinner" ) @@ -80,12 +81,31 @@ func autoProvisionBackend( progressMsg := fmt.Sprintf("Provisioning %s backend `%s` for `%s` in stack `%s`", strings.ToUpper(backendType), backendName, component, stack) completedMsg := fmt.Sprintf("Provisioned %s backend `%s` for `%s` in stack `%s`", strings.ToUpper(backendType), backendName, component, stack) - return spinner.ExecWithSpinner(progressMsg, completedMsg, func() error { + // Capture provisioning result to display warnings after spinner completes. + // Warnings must be displayed AFTER the spinner to avoid concurrent output corruption. + var result *backend.ProvisionResult + err = spinner.ExecWithSpinner(progressMsg, completedMsg, func() error { ctx, cancel := context.WithTimeout(ctx, 5*time.Minute) defer cancel() - return createFunc(ctx, atmosConfig, backendConfig, authContext) + var createErr error + result, createErr = createFunc(ctx, atmosConfig, backendConfig, authContext) + return createErr }) + if err != nil { + return err + } + + // Display warnings AFTER spinner completes to avoid concurrent output issues. + // The spinner runs operations in a background goroutine while animating on stderr, + // so any output during spinner execution would interleave and corrupt the display. + if result != nil { + for _, warning := range result.Warnings { + _ = ui.Warning(warning) + } + } + + return nil } // isBackendProvisionEnabled checks if provision.backend.enabled is true. diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go index 447cd00739..4013d2c314 100644 --- a/pkg/provisioner/provisioner.go +++ b/pkg/provisioner/provisioner.go @@ -11,6 +11,7 @@ import ( "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/provisioner/backend" "github.com/cloudposse/atmos/pkg/schema" + "github.com/cloudposse/atmos/pkg/ui" "github.com/cloudposse/atmos/pkg/ui/spinner" ) @@ -86,7 +87,10 @@ func ProvisionWithParams(params *ProvisionParams) error { progressMsg := fmt.Sprintf("Provisioning %s backend `%s` for `%s` in stack `%s`", strings.ToUpper(backendType), backendName, params.Component, params.Stack) completedMsg := fmt.Sprintf("Provisioned %s backend `%s` for `%s` in stack `%s`", strings.ToUpper(backendType), backendName, params.Component, params.Stack) - return spinner.ExecWithSpinner(progressMsg, completedMsg, func() error { + // Capture provisioning result to display warnings after spinner completes. + // Warnings must be displayed AFTER the spinner to avoid concurrent output corruption. + var result *backend.ProvisionResult + err = spinner.ExecWithSpinner(progressMsg, completedMsg, func() error { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() @@ -94,8 +98,24 @@ func ProvisionWithParams(params *ProvisionParams) error { // This enables in-process SDK calls with Atmos-managed credentials. // The AuthContext was populated by the command layer through InitConfigAndAuth, // which merges component-level auth with global auth and respects default identity settings. - return backend.ProvisionBackend(ctx, params.AtmosConfig, componentConfig, params.AuthContext) + var provErr error + result, provErr = backend.ProvisionBackend(ctx, params.AtmosConfig, componentConfig, params.AuthContext) + return provErr }) + if err != nil { + return err + } + + // Display warnings AFTER spinner completes to avoid concurrent output issues. + // The spinner runs operations in a background goroutine while animating on stderr, + // so any output during spinner execution would interleave and corrupt the display. + if result != nil { + for _, warning := range result.Warnings { + _ = ui.Warning(warning) + } + } + + return nil } // ListBackends lists all backends in a stack. diff --git a/pkg/provisioner/provisioner_test.go b/pkg/provisioner/provisioner_test.go index a91a34aecd..9c6700ae6e 100644 --- a/pkg/provisioner/provisioner_test.go +++ b/pkg/provisioner/provisioner_test.go @@ -89,7 +89,7 @@ func TestProvisionWithParams_BackendProvisioningSuccess(t *testing.T) { // Register a mock backend provisioner for testing. mockProvisionerCalled := false - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*backend.ProvisionResult, error) { mockProvisionerCalled = true // Verify the backend config was passed correctly. bucket, ok := backendConfig["bucket"].(string) @@ -100,7 +100,7 @@ func TestProvisionWithParams_BackendProvisioningSuccess(t *testing.T) { assert.True(t, ok) assert.Equal(t, "us-west-2", region) - return nil + return &backend.ProvisionResult{}, nil } // Temporarily register the mock provisioner. @@ -143,8 +143,8 @@ func TestProvisionWithParams_BackendProvisioningFailure(t *testing.T) { t.Cleanup(backend.ResetRegistryForTesting) // Register a mock backend provisioner that fails. - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return errors.New("provisioning failed: bucket already exists in another account") + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*backend.ProvisionResult, error) { + return nil, errors.New("provisioning failed: bucket already exists in another account") } // Temporarily register the mock provisioner. @@ -208,9 +208,9 @@ func TestProvision_DelegatesToProvisionWithParams(t *testing.T) { // Register a mock backend provisioner. mockProvisionerCalled := false - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*backend.ProvisionResult, error) { mockProvisionerCalled = true - return nil + return &backend.ProvisionResult{}, nil } backend.RegisterBackendCreate("s3", mockProvisioner) @@ -250,10 +250,10 @@ func TestProvisionWithParams_WithAuthContext(t *testing.T) { } // Register a mock backend provisioner that verifies authContext handling. - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*backend.ProvisionResult, error) { // AuthContext is passed through from params; nil here because test provides nil. assert.Nil(t, authContext, "AuthContext should be nil when params.AuthContext is nil") - return nil + return &backend.ProvisionResult{}, nil } backend.RegisterBackendCreate("s3", mockProvisioner) @@ -324,8 +324,8 @@ func TestProvisionWithParams_BackendTypeValidation(t *testing.T) { // Register a mock provisioner for backend type. if tt.provisionType == "backend" { - mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) error { - return nil + mockProvisioner := func(ctx context.Context, atmosConfig *schema.AtmosConfiguration, backendConfig map[string]any, authContext *schema.AuthContext) (*backend.ProvisionResult, error) { + return &backend.ProvisionResult{}, nil } backend.RegisterBackendCreate("s3", mockProvisioner) } diff --git a/pkg/ui/formatter.go b/pkg/ui/formatter.go index d059716067..8d62abaddf 100644 --- a/pkg/ui/formatter.go +++ b/pkg/ui/formatter.go @@ -542,6 +542,46 @@ func trimRightSpaces(s string) string { return result.String() } +// trimLeftSpaces removes only leading spaces from an ANSI-coded string while +// preserving all ANSI escape sequences on the remaining content. +// This is useful for removing Glamour's paragraph indent while preserving styled content. +func trimLeftSpaces(s string) string { + stripped := ansi.Strip(s) + trimmed := strings.TrimLeft(stripped, " ") + + if trimmed == stripped { + return s // No leading spaces to remove. + } + if trimmed == "" { + return "" // All spaces. + } + + // Calculate how many leading spaces to skip. + leadingSpaces := len(stripped) - len(trimmed) + + // Walk through original string, skipping ANSI codes and counting spaces. + spacesSkipped := 0 + i := 0 + + // Skip leading ANSI codes and spaces until we've skipped the required amount. +skipLoop: + for i < len(s) && spacesSkipped < leadingSpaces { + switch { + case isANSIStart(s, i): + // Skip ANSI sequence (don't output it since it's styling skipped content). + i = skipANSISequence(s, i) + case s[i] == ' ': + spacesSkipped++ + i++ + default: + break skipLoop // Non-space content found. + } + } + + // Return remaining content (including any ANSI codes). + return s[i:] +} + // isWhitespace checks if byte b is a space or tab. func isWhitespace(b byte) bool { return b == ' ' || b == '\t' @@ -680,13 +720,15 @@ func (f *formatter) toastMarkdown(icon string, style *lipgloss.Style, text strin if len(lines) == 1 { // For single line: trim leading spaces from Glamour's paragraph indent - // since the icon+space already provides visual separation - line := strings.TrimLeft(lines[0], space) + // since the icon+space already provides visual separation. + // Use ANSI-aware trimming since Glamour may wrap spaces in color codes. + line := trimLeftSpaces(lines[0]) return fmt.Sprintf(iconMessageFormat, styledIcon, line), nil } - // Multi-line: trim leading spaces from first line (goes next to icon) - lines[0] = strings.TrimLeft(lines[0], space) + // Multi-line: trim leading spaces from first line (goes next to icon). + // Use ANSI-aware trimming since Glamour may wrap spaces in color codes. + lines[0] = trimLeftSpaces(lines[0]) // Multi-line: first line with icon, rest indented to align under first line's text result := fmt.Sprintf(iconMessageFormat, styledIcon, lines[0]) @@ -697,8 +739,9 @@ func (f *formatter) toastMarkdown(icon string, style *lipgloss.Style, text strin indent := strings.Repeat(space, iconWidth+1) // +1 for the space in "%s %s" format for i := 1; i < len(lines); i++ { - // Glamour already added 2-space paragraph indent, replace with our calculated indent - line := strings.TrimLeft(lines[i], space) // Remove Glamour's indent + // Glamour already added 2-space paragraph indent, replace with our calculated indent. + // Use ANSI-aware trimming since Glamour may wrap spaces in color codes. + line := trimLeftSpaces(lines[i]) result += newline + indent + line } @@ -834,11 +877,13 @@ func (f *formatter) renderInlineMarkdownWithBase(text string, baseStyle *lipglos } // For single line, trim leading spaces. + // Use ANSI-aware trimming since Glamour may wrap spaces in color codes. if len(lines) == 1 { - rendered = strings.TrimLeft(lines[0], space) + rendered = trimLeftSpaces(lines[0]) } else { // Multi-line: trim first line and rejoin. - lines[0] = strings.TrimLeft(lines[0], space) + // Use ANSI-aware trimming since Glamour may wrap spaces in color codes. + lines[0] = trimLeftSpaces(lines[0]) rendered = strings.Join(lines, newline) } diff --git a/pkg/ui/formatter_test.go b/pkg/ui/formatter_test.go index b85ce5183b..d12d92c195 100644 --- a/pkg/ui/formatter_test.go +++ b/pkg/ui/formatter_test.go @@ -1710,3 +1710,160 @@ func TestTrimRight(t *testing.T) { }) } } + +//nolint:dupl // Test structure intentionally mirrors TestTrimRight for consistency. +func TestTrimLeftSpaces(t *testing.T) { + tests := []struct { + name string + input string + expected string + desc string + }{ + { + name: "plain text no leading spaces", + input: "hello world", + expected: "hello world", + desc: "Baseline: plain text without leading spaces should be unchanged", + }, + { + name: "plain text with leading spaces", + input: " hello world", + expected: "hello world", + desc: "Plain text with leading spaces should be trimmed", + }, + { + name: "ANSI colored text no leading spaces", + input: "\x1b[38;2;247;250;252mhello world\x1b[0m", + expected: "\x1b[38;2;247;250;252mhello world\x1b[0m", + desc: "ANSI colored text without leading spaces should preserve all codes", + }, + { + name: "ANSI colored text with plain leading spaces", + input: " \x1b[38;2;247;250;252mhello world\x1b[0m", + expected: "\x1b[38;2;247;250;252mhello world\x1b[0m", + desc: "ANSI colored text with plain leading spaces should trim spaces", + }, + { + name: "ANSI codes before leading spaces (Glamour pattern)", + input: "\x1b[38;2;247;250;252m\x1b[0m\x1b[38;2;247;250;252m\x1b[0m \x1b[38;2;247;250;252mhello world\x1b[0m", + expected: "\x1b[38;2;247;250;252mhello world\x1b[0m", + desc: "ANSI codes before leading spaces (Glamour pattern) should be trimmed", + }, + { + name: "ANSI wrapped leading spaces", + input: "\x1b[38;2;247;250;252m \x1b[0m\x1b[38;2;247;250;252mhello world\x1b[0m", + expected: "\x1b[0m\x1b[38;2;247;250;252mhello world\x1b[0m", + desc: "ANSI-wrapped leading spaces should be trimmed (reset code preserved)", + }, + { + name: "mixed ANSI codes and spaces at start", + input: "\x1b[0m\x1b[38;2;247;250;252m\x1b[0m \x1b[38;2;247;250;252m• Item one\x1b[0m", + expected: "\x1b[38;2;247;250;252m• Item one\x1b[0m", + desc: "Mixed ANSI codes and spaces at start should be trimmed correctly", + }, + { + name: "Unicode characters with leading spaces", + input: " ℹ hello → world", + expected: "ℹ hello → world", + desc: "Unicode characters with leading spaces should be trimmed correctly", + }, + { + name: "Unicode with ANSI and leading spaces", + input: "\x1b[38;2;247;250;252m \x1b[0m\x1b[38;2;247;250;252mℹ hello → world\x1b[0m", + expected: "\x1b[0m\x1b[38;2;247;250;252mℹ hello → world\x1b[0m", + desc: "Unicode with ANSI codes and leading spaces should trim correctly (reset code preserved)", + }, + { + name: "empty string", + input: "", + expected: "", + desc: "Empty string should remain empty", + }, + { + name: "only spaces", + input: " ", + expected: "", + desc: "String with only spaces should become empty", + }, + { + name: "only ANSI wrapped spaces", + input: "\x1b[38;2;247;250;252m \x1b[0m", + expected: "", + desc: "String with only ANSI-wrapped spaces should become empty", + }, + { + name: "preserves trailing spaces", + input: " hello world ", + expected: "hello world ", + desc: "Trailing spaces should be preserved, only leading removed", + }, + { + name: "preserves ANSI on trailing spaces", + input: "\x1b[38;2;247;250;252m \x1b[0m\x1b[38;2;247;250;252mhello world\x1b[0m\x1b[38;2;247;250;252m \x1b[0m", + expected: "\x1b[0m\x1b[38;2;247;250;252mhello world\x1b[0m\x1b[38;2;247;250;252m \x1b[0m", + desc: "ANSI codes on trailing spaces should be preserved (leading reset code after trim)", + }, + { + name: "real Glamour output with bullet", + input: "\x1b[38;2;247;250;252m\x1b[0m\x1b[38;2;247;250;252m\x1b[0m \x1b[38;2;247;250;252m• \x1b[0m\x1b[38;2;247;250;252mItem one\x1b[0m", + expected: "\x1b[38;2;247;250;252m• \x1b[0m\x1b[38;2;247;250;252mItem one\x1b[0m", + desc: "Real Glamour bullet list output should have leading spaces trimmed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := trimLeftSpaces(tt.input) + + // Compare results. + if result != tt.expected { + t.Errorf("\nTest: %s\nDescription: %s\n\nInput:\n Raw: %q\n Hex: % X\n Visual: %s\n\nExpected:\n Raw: %q\n Hex: % X\n Visual: %s\n\nGot:\n Raw: %q\n Hex: % X\n Visual: %s", + tt.name, + tt.desc, + tt.input, + []byte(tt.input), + tt.input, + tt.expected, + []byte(tt.expected), + tt.expected, + result, + []byte(result), + result, + ) + } + + // Additional verification: check visual width. + strippedInput := ansi.Strip(tt.input) + strippedExpected := ansi.Strip(tt.expected) + strippedResult := ansi.Strip(result) + + expectedWidth := ansi.StringWidth(strings.TrimLeft(strippedInput, " ")) + resultWidth := ansi.StringWidth(strippedResult) + + if resultWidth != expectedWidth { + t.Errorf("\nVisual width mismatch:\n Expected trimmed width: %d (from %q)\n Got width: %d (from %q)", + expectedWidth, + strings.TrimLeft(strippedInput, " "), + resultWidth, + strippedResult, + ) + } + + // Verify no leading whitespace in result. + if strippedResult != strings.TrimLeft(strippedResult, " ") { + t.Errorf("\nResult still has leading whitespace:\n Stripped result: %q\n After TrimLeft: %q", + strippedResult, + strings.TrimLeft(strippedResult, " "), + ) + } + + // Verify expected also matches this property. + if strippedExpected != strings.TrimLeft(strippedExpected, " ") { + t.Errorf("\nTest case error - expected value has leading whitespace:\n Stripped expected: %q\n After TrimLeft: %q", + strippedExpected, + strings.TrimLeft(strippedExpected, " "), + ) + } + }) + } +}