Set S3 Storage Region in Package Manager and Chronicle

[timtalbot]

Summary

New versions of Package Manager now require that the AWS region be set in storage config when using s3 storage. This sets the region and also addresses a previously hard-coded region in the Chronicle s3 storage config.

Changes

Core functionality

• api/product/util.go: Added GetAWSRegion() utility function (moved from secret.go) that:
  • Checks AWS_REGION environment variable first
  • Falls back to AWS_DEFAULT_REGION if not set
  • Defaults to us-east-2 for backwards compatibility
• api/product/secret.go: Refactored to use the new shared GetAWSRegion() function
• internal/controller/core/site_controller_package_manager.go: Now sets pm.Spec.Config.S3Storage.Region using GetAWSRegion()
• internal/controller/core/site_controller_chronicle.go: Replaced hardcoded us-east-2 with GetAWSRegion() for consistency

Testing

• api/product/util_test.go: Added test coverage for GetAWSRegion() including:
  • AWS_REGION environment variable takes precedence
  • AWS_DEFAULT_REGION fallback behavior
  • Default region when no environment variables are set
  • Proper environment variable restoration in tests

Housekeeping

• .gitignore: Added bin/ directory to ignore build artifacts
• .claude/settings.json: Updated build command pattern (fixes warning in Claude CLI: "Bash(make *)": Use ":*" for prefix matching, not just "*". Change to "Bash(make :*)" for prefix matching. Examples: Bash(npm run:*), Bash(git:*))

[claude]

Claude finished @timtalbot's task —— View job

---

Reviewing PR #48

• Read review guidelines
• Get PR diff and analyze changes
• Create pending review with inline comments
• Submit review

Reviewed this PR. The refactoring is clean and follows existing patterns. Left two minor suggestions:

1. Update GetAWSRegion() doc comment to reflect broader usage beyond secrets
2. Consider using t.Setenv() to simplify test cleanup logic

Overall a solid fix for the Package Manager region requirement.

[claude]

Review Summary

Clean refactoring that addresses an important compatibility issue. The changes follow existing patterns and include proper test coverage.

Feedback

api/product/util.go:95 - The doc comment references "secret operations" but GetAWSRegion() is now used more broadly for storage configs. Consider generalizing:

// GetAWSRegion returns the AWS region to use for AWS services.

api/product/util_test.go:100-116 - The environment variable save/restore logic could be simplified using t.Setenv() which automatically cleans up after each test (Go 1.17+):

t.Run(tt.name, func(t *testing.T) {
    if tt.awsRegion != "" {
        t.Setenv("AWS_REGION", tt.awsRegion)
    }
    if tt.awsDefault != "" {
        t.Setenv("AWS_DEFAULT_REGION", tt.awsDefault)
    }
    if got := product.GetAWSRegion(); got != tt.want {
        t.Errorf("GetAWSRegion() = %v, want %v", got, tt.want)
    }
})

Checklist

Controller Changes:

• Config flows from Site → Product correctly
• Changes are idempotent (region is read from env each reconcile)

Security:

• No credential handling concerns - region is not sensitive data
• Using standard AWS SDK region constants

The default to us-east-2 for backwards compatibility is reasonable. Overall this is a solid fix.