fix: Fix backend CRUD issues - create visibility, error messages, and false success bug#1960
fix: Fix backend CRUD issues - create visibility, error messages, and false success bug#1960
Conversation
… false success bug - Fix `create` command not appearing in subcommands list (Use field was "<component>") - Add ErrorBuilder pattern for missing --stack flag error (shows "stack flag is required" + hint) - Add ErrProvisioningNotConfigured sentinel error to errors/errors.go - Fix false success bug: ProvisionBackend now returns error when provisioning not configured - Update all affected tests to expect errors instead of nil returns - Improve test comments for clarity Fixes: - Backend create command is now visible in help output - Error messages now show which flag is required and how to specify it - Commands no longer report success when provisioning configuration is missing (strict behavior) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCommands for Terraform backend were refactored to use a prompting-capable parser and centralized execute* helpers; positional component args changed from required ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Parser
participant ConfigInit
participant ProvisionRegistry
participant BackendProvider
participant Spinner
CLI->>Parser: Parse(ctx, args) => Result{Component, Stack, Identity, Format}
Parser-->>CLI: Result
CLI->>ConfigInit: InitConfigAndAuth(component, stack, identity)
ConfigInit-->>CLI: AtmosConfig, AuthContext
CLI->>ProvisionRegistry: GetBackendCreate(backendType)
ProvisionRegistry-->>CLI: createFunc / nil
CLI->>ProvisionRegistry: BackendName(backendType, backendConfig)
ProvisionRegistry-->>CLI: backendName
CLI->>Spinner: ExecWithSpinner(ctx, "creating backend...", timeout, func)
Spinner->>BackendProvider: createFunc(ctx, atmosConfig, backendConfig, authContext)
BackendProvider-->>Spinner: success / error
Spinner-->>CLI: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/*_test.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/{pkg,internal,cmd}/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2024-12-17T07:10:26.295ZApplied to files:
📚 Learning: 2025-12-10T18:32:51.237ZApplied to files:
📚 Learning: 2025-12-21T04:10:29.030ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-11-10T23:23:39.771ZApplied to files:
🧬 Code graph analysis (1)pkg/provisioner/backend/backend_test.go (2)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
==========================================
+ Coverage 74.37% 74.42% +0.04%
==========================================
Files 775 776 +1
Lines 70359 70649 +290
==========================================
+ Hits 52330 52579 +249
- Misses 14592 14616 +24
- Partials 3437 3454 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add interactive prompting support to backend commands when component positional arg or --stack flag is missing. Uses StandardParser's built-in prompting infrastructure with PositionalArgsBuilder and completion prompt options. Changes: - Add WithCompletionPrompt for stack flag prompting - Add WithPositionalArgPrompt for component prompting - Change Use syntax from <component> to [component] to indicate optional - Simplify helper functions by removing unused cmd/perfLabel params - Update tests for new function signatures Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add cmd/terraform/backend/backend_create.go to flag-handler agent's reference implementations. It demonstrates both flag prompting (WithCompletionPrompt) and positional arg prompting (WithPositionalArgPrompt). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add s3_integration_test.go with gofakes3 in-memory S3 server for testing low-level bucket operations (create, delete, versioning, object management) - Add s3_e2e_test.go with end-to-end tests that exercise production code paths (CreateS3Backend, DeleteS3Backend) using mock-aws provider - Add s3SkipUnsupportedTestOps flag to skip encryption/public-access/tags operations not supported by gofakes3 (test-only, not exposed in YAML) - Update mock-aws identity PostAuthenticate to use 3-step pattern: SetupFiles, SetAuthContext, SetEnvironmentVariables - Update s3_delete.go to use s3ClientFactory for test injection Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/provisioner/backend/s3_e2e_test.go`:
- Around line 17-33: The cleanup in setupFakeS3Factory does not restore the S3
client factory, risking test pollution; update the t.Cleanup closure in
setupFakeS3Factory to call ResetS3ClientFactory() in addition to
SetS3SkipUnsupportedTestOps(false) so both the fake factory set by
SetS3ClientFactory(...) and the skip flag set by
SetS3SkipUnsupportedTestOps(true) are reset after the test.
In `@pkg/provisioner/backend/s3.go`:
- Around line 40-60: s3ClientFactory is a package-level mutable variable and
needs synchronization for parallel tests; add a package-level sync.RWMutex (e.g.
s3ClientFactoryMu) and protect writes to s3ClientFactory in SetS3ClientFactory
and ResetS3ClientFactory with s3ClientFactoryMu.Lock()/Unlock(), and protect
reads where the factory is invoked (any call sites that call
s3ClientFactory(cfg)) with s3ClientFactoryMu.RLock()/RUnlock() to avoid data
races.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/auth/providers/mock/aws/identity.gopkg/provisioner/backend/s3.gopkg/provisioner/backend/s3_delete.gopkg/provisioner/backend/s3_e2e_test.gopkg/provisioner/backend/s3_integration_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter) in Go code
Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (cfg,log,u,errUtils)
All errors MUST be wrapped using static errors defined inerrors/errors.go- useerrors.Joinfor combining errors,fmt.Errorfwith%wfor context, anderrors.Is()for error checking
Never manually create mocks - usego.uber.org/mock/mockgenwith//go:generatedirectives in Go code
Keep files small and focused - under 600 lines with one cmd/impl per file, co-locate tests, never use//revive:disable:file-length-limit
Use colors frompkg/ui/theme/colors.gofor all UI theming in Go code
Code must be compatible with Linux, macOS, and Windows - use SDKs over binaries, usefilepath.Join()instead of h...
Files:
pkg/provisioner/backend/s3_delete.gopkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.gopkg/auth/providers/mock/aws/identity.gopkg/provisioner/backend/s3.go
**/{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Add
defer perf.Track(atmosConfig, "pkg.FuncName")()plus blank line to all public functions, usingnilif no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Files:
pkg/provisioner/backend/s3_delete.gopkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.gopkg/auth/providers/mock/aws/identity.gopkg/provisioner/backend/s3.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixturesPrefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with
go.uber.org/mock/mockgen, use table-driven tests, target >80% coverage
Files:
pkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.go
{go.mod,go.sum}
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Files:
go.mod
🧠 Learnings (20)
📓 Common learnings
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:53.976Z
Learning: When `atmos terraform clean --everything` is used without specifying a component and without the `--force` flag, prompt the user for confirmation before deleting all components. Use the `--force` flag to skip the confirmation prompt.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1907
File: demos/fixtures/acme/stacks/catalog/api.yaml:1-29
Timestamp: 2026-01-09T04:49:35.038Z
Learning: In the cloudposse/atmos demos/fixtures, components can provide Terraform outputs via `remote_state_backend.static` configuration blocks instead of traditional Terraform output blocks. This pattern is used for demo/fixture purposes to simulate cross-component state references without deploying actual infrastructure. The `!terraform.state` YAML function reads from these static backends.
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform.go:114-118
Timestamp: 2024-10-21T17:51:07.087Z
Learning: Use `bubbletea` for confirmation prompts instead of `fmt.Scanln` in the `atmos terraform clean` command.
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/provisioner/backend/s3_delete.gopkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.gopkg/auth/providers/mock/aws/identity.gopkg/provisioner/backend/s3.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests - use interfaces and dependency injection for testability, generate mocks with `go.uber.org/mock/mockgen`, use table-driven tests, target >80% coverage
Applied to files:
pkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/provisioner/backend/s3_integration_test.gopkg/provisioner/backend/s3_e2e_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/provisioner/backend/s3_integration_test.gogo.mod
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to {go.mod,go.sum} : Manage dependencies with Go modules and keep dependencies up to date while minimizing external dependencies
Applied to files:
go.mod
📚 Learning: 2024-11-18T13:59:10.824Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: Go version 1.23.0 was deliberately introduced by the maintainer (aknysh) in January 2025. While this might be a pre-release or development version of Go, it has been approved for use in this project.
Applied to files:
go.mod
📚 Learning: 2025-01-17T00:21:32.987Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:3-3
Timestamp: 2025-01-17T00:21:32.987Z
Learning: The project uses Go version 1.23.0 which has been confirmed by the maintainer to be working in production for months. Do not flag this as an invalid Go version.
Applied to files:
go.mod
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
go.modpkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
pkg/provisioner/backend/s3_e2e_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
pkg/provisioner/backend/s3_e2e_test.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-11-09T19:06:58.470Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1752
File: pkg/profile/list/formatter_table.go:27-29
Timestamp: 2025-11-09T19:06:58.470Z
Learning: In the cloudposse/atmos repository, performance tracking with `defer perf.Track()` is enforced on all functions via linting, including high-frequency utility functions, formatters, and renderers. This is a repository-wide policy to maintain consistency and avoid making case-by-case judgment calls about which functions should have profiling.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/{pkg,internal,cmd}/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` plus blank line to all public functions, using `nil` if no atmosConfig param - exceptions: trivial getters/setters, command constructors, simple factories, functions delegating to tracked functions
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-11-30T04:16:24.155Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1821
File: pkg/merge/deferred.go:34-48
Timestamp: 2025-11-30T04:16:24.155Z
Learning: In the cloudposse/atmos repository, the `defer perf.Track()` guideline applies to functions that perform meaningful work (I/O, computation, external calls), but explicitly excludes trivial accessors/mutators (e.g., simple getters, setters with single integer increments, string joins, or map appends) where the tracking overhead would exceed the actual method cost and provide no actionable performance data. Hot-path methods called in tight loops should especially avoid perf.Track() if they perform only trivial operations.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2026-01-04T00:55:21.720Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T00:55:21.720Z
Learning: Applies to **/*.go : Organize imports into three groups separated by blank lines, sorted alphabetically: Go stdlib, 3rd-party (NOT cloudposse/atmos), then Atmos packages with maintained aliases (`cfg`, `log`, `u`, `errUtils`)
Applied to files:
pkg/auth/providers/mock/aws/identity.go
📚 Learning: 2025-05-30T03:21:37.197Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1274
File: go.mod:63-63
Timestamp: 2025-05-30T03:21:37.197Z
Learning: The redis dependency (github.com/redis/go-redis/v9) in the atmos project is only used in tests, not in production code.
Applied to files:
pkg/auth/providers/mock/aws/identity.go
🧬 Code graph analysis (3)
pkg/provisioner/backend/s3_integration_test.go (1)
pkg/provisioner/backend/s3.go (1)
ResetS3ClientFactory(54-60)
pkg/provisioner/backend/s3_e2e_test.go (2)
pkg/provisioner/backend/s3.go (4)
SetS3ClientFactory(47-51)S3ClientAPI(28-38)SetS3SkipUnsupportedTestOps(68-72)CreateS3Backend(99-144)pkg/schema/schema.go (1)
AuthContext(665-675)
pkg/provisioner/backend/s3.go (1)
pkg/perf/perf.go (1)
Track(121-138)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (19)
go.mod (2)
74-74: LGTM - Good choice for S3 testing.The
gofakes3library provides an in-memory S3 server, enabling integration tests without AWS infrastructure. This aligns with the test isolation patterns used elsewhere in the codebase.
346-346: Indirect dependencies look fine.These are transitive requirements from
gofakes3(goskiplist) and coverage tooling (gocovmerge).Also applies to: 391-391
pkg/provisioner/backend/s3.go (3)
62-72: Test flag pattern is reasonable.The
s3SkipUnsupportedTestOpsflag cleanly handles gofakes3 limitations without polluting production code paths. The naming and comment make the intent clear.
127-128: Factory injection applied correctly.Good use of the factory pattern here - allows tests to inject mock clients while production code gets the real S3 client.
315-319: Early return is intentional and already covered by unit tests.The early return when
s3SkipUnsupportedTestOpsis true skips operations unsupported by gofakes3 during integration testing. However, encryption, public access blocking, and tagging are already tested separately with mocks ins3_test.go(lines 568–661), covering both success and failure cases. No additional tests needed.Likely an incorrect or invalid review comment.
pkg/provisioner/backend/s3_delete.go (1)
85-85: Factory pattern applied consistently.Good alignment with
s3.go- deletion now uses the same injectable factory, enabling tests to verify deletion paths with fake S3.pkg/provisioner/backend/s3_integration_test.go (4)
206-251: Good versioning prerequisite documented.The comment on line 207-208 explaining that versioning must be enabled for
ListObjectVersionsto return objects is helpful. This documents a real AWS behavior that could otherwise cause confusion.
331-355: Multi-region tests provide good coverage.Testing bucket creation across multiple regions validates the
LocationConstraintlogic for us-east-1 vs other regions.
60-74: Solid integration test structure.Tests are well-organized with clear naming, proper setup/cleanup, and good use of
requirefor critical assertions vsassertfor expected outcomes.
26-58: Factory integration is tested separately in s3_e2e_test.go—no change needed.The direct client creation here is intentional. The integration tests exercise internal functions directly, while
s3_e2e_test.gohassetupFakeS3Factory()that properly injects the fake client viaSetS3ClientFactory(). This two-file approach tests both the unit-level backend logic and the production code path that uses the factory.pkg/auth/providers/mock/aws/identity.go (3)
12-16: Import organization follows guidelines.The
awsCloudalias is clear and follows the Atmos import grouping convention.
140-189: Three-step pattern aligns with real AWS providers.Good refactor. The mock identity now exercises the same credential setup flow as production AWS identities, improving test fidelity. The conditional checks for
params != nilbefore accessing nested fields are correct.
191-207: Backwards compatibility preserved with legacy format.The dual-write approach (AWS-format files + legacy JSON) maintains compatibility with existing tests using
LoadCredentials. The comment on lines 191-192 explains the rationale clearly.pkg/provisioner/backend/s3_e2e_test.go (6)
35-93: Solid E2E coverage for the create flow.Test structure is clean: env setup, fake S3, auth flow, production path, verification. Using
t.Setenvandt.TempDirensures proper isolation.One note:
CreateS3Backend(ctx, nil, backendConfig, authContext)passesnilforatmosConfig. This works if the implementation handles it gracefully (perf.Track accepts nil per coding guidelines), but worth confirming.
95-156: Good delete lifecycle coverage.Tests the complete create → verify → delete → verify-deleted flow. Proper use of
requirefor preconditions andassertfor final verification.
158-176: Factory injection test works as intended.Comment clarifies this tests S3 client factory injection rather than the full production path with nil auth. Validates the test infrastructure itself.
178-196: Clean idempotency verification.Tests the essential behavior: first call creates and returns
false, second call detects existence and returnstrue.
198-247: Validates env var propagation correctly.Covers the critical AWS environment variables that downstream processes need. Type assertions with
okcheck is defensive.
249-320: Good isolation test for multi-identity scenarios.Validates that different identities get separate credential/config files. The bucket creation at lines 305-310 uses the shared fake client directly, which is fine since the goal is verifying file isolation, not auth-path bucket creation (already covered above).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add sync.RWMutex protection for s3ClientFactory and s3SkipUnsupportedTestOps to prevent data races during parallel test execution - Add getS3ClientFactory() and getS3SkipUnsupportedTestOps() thread-safe getters - Update all read sites to use thread-safe getters - Add ResetS3ClientFactory() to E2E test cleanup to prevent test pollution Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `terraform.provision` section defined at the stack level (e.g., in `_defaults.yaml`) was not being merged into component configurations. This caused `provision.backend.enabled: true` to be ignored when set globally, requiring users to duplicate the setting in every component. Changes: - Add ErrInvalidTerraformProvision error sentinel - Add BaseComponentProvisionSection to schema.BaseComponentConfig - Add GlobalProvisionSection to ComponentProcessorOptions - Extract global terraform.provision in stack processor - Merge provision from global → base component → component levels The fix follows the same pattern used for the `source` section, which already worked correctly for global inheritance. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add automatic backend provisioning when `provision.backend.enabled: true` is set. Backends are now created just-in-time during terraform init, eliminating the need for a separate `backend create` command. Changes: - Add BackendExists registry for idempotent provisioning checks - Add BackendName registry so provisioner doesn't know backend internals - Add S3BackendExists using HeadBucket for existence checks - Add S3BackendName to extract bucket name from config - Add backend_hook.go with before.terraform.init hook registration - Replace multi-line ui.Info/Success with single-line spinner feedback - Use backticks for markdown-friendly terminal rendering The auto-provisioning is idempotent: first run creates with spinner, subsequent runs silently skip if backend exists. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Blog posts are served at /changelog/ not /blog/ in Docusaurus config. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
These changes were released in v1.204.0-rc.5. |
2 participants
what
createcommand not appearing in subcommands list (Use field was wrong)--stackflag with helpful contextErrProvisioningNotConfiguredsentinel error to errors/errors.gowhy
The backend commands had multiple issues:
createsubcommand wasn't visible in help becauseUse: "<component>"doesn't include the command namecreatewithout provisioning configuration would show "Successfully provisioned" even though nothing was done (false success)These fixes ensure the backend commands are discoverable, errors are helpful and actionable, and operations fail-fast with clear context.
references
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.