fix: Do not add configured prefix for !store.get calls
#1814
Conversation
📝 WalkthroughWalkthroughThe PR removes prefix-based key transformation from GetKey methods across multiple store backends (Artifactory, AWS SSM Parameter Store, Google Secret Manager, Redis). Each backend now uses provided keys directly rather than prepending the store's configured prefix. Corresponding test cases are updated to reflect this behavior change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/store/redis_store_test.go (1)
427-445: Tests correctly enforce raw-key usage; fix comment punctuation.The expectation against
tt.key(without prefix) is a solid check of the newGetKeycontract. The new comment should end with a period for godot:- // GetKey uses the key exactly as provided without adding prefix + // GetKey uses the key exactly as provided without adding prefix.pkg/store/artifactory_store.go (1)
196-213: ArtifactoryStore.GetKey now matches!store.getsemantics.Using the provided
keydirectly as the repo file path (only joining withrepoName) is aligned with the documented behavior and new tests; the added comments read well and are accurate.One small cleanup to consider later: after
totalDownloaded, totalExpected, err := s.rtManager.DownloadFiles(downloadParams) if err != nil { return nil, fmt.Errorf(errWrapFormat, ErrDownloadFile, err) }the subsequent check
// Only check for mismatch if there was an error if err != nil && totalDownloaded != totalExpected { return nil, fmt.Errorf(errWrapFormat, ErrDownloadFile, err) }can never fire because
err != nilhas already been handled. If you do want to fail on count mismatch, you can drop theerr != nilpart; otherwise, consider removing the block entirely.Also applies to: 291-349
pkg/store/artifactory_store_test.go (1)
349-438: GetKey tests capture the new contract; tighten up a couple of nits.The scenarios here nicely verify that
GetKey:
- Uses the key as-is (no prefix),
- Appends
.jsonwhen needed,- And properly propagates download errors.
Two small cleanups to keep things sharp:
expectedPathisn’t used anywhere in the table or assertions; you can safely drop it to avoid confusion:- expectedPath string expectError bool errorContains string @@ - expectedPath: "test-repo/config/app-settings.json", expectError: false, @@ - expectedPath: "test-repo/my-secret.json", expectError: false, @@ - expectedPath: "test-repo/config.json", expectError: false,
- The new inline comment should end with a period for godot:
- // The key should be used exactly as provided, WITHOUT the prefix prepended + // The key should be used exactly as provided, WITHOUT the prefix prepended.pkg/store/google_secret_manager_store_test.go (1)
682-734: GSM GetKey tests solidly cover prefix-less behavior; fix comment punctuation.The two additions here do a good job of locking in that
GetKey:
- Uses the key verbatim for the secret name, and
- Ignores any configured
prefix.One tiny style fix to keep godot happy in both places:
- // The key should be used exactly as provided, WITHOUT the prefix prepended + // The key should be used exactly as provided, WITHOUT the prefix prepended.Also applies to: 736-768
📜 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 selected for processing (8)
pkg/store/artifactory_store.go(1 hunks)pkg/store/artifactory_store_test.go(1 hunks)pkg/store/aws_ssm_param_store.go(1 hunks)pkg/store/aws_ssm_param_store_test.go(9 hunks)pkg/store/google_secret_manager_store.go(1 hunks)pkg/store/google_secret_manager_store_test.go(2 hunks)pkg/store/redis_store.go(1 hunks)pkg/store/redis_store_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Use functional options pattern to avoid functions with many parameters. Define Option functions that modify a Config struct and pass variadic options to New functions.
Use context.Context for cancellation signals, deadlines/timeouts, and request-scoped values (trace IDs). Context should be first parameter in functions that accept it. DO NOT use context for configuration, dependencies, or avoiding proper function parameters.
All comments must end with periods. This is enforced by the godot linter.
NEVER delete existing comments without a very strong reason. Preserve helpful comments, update them to match code changes, refactor for clarity, and add context when modifying code. Only remove factually incorrect, duplicated, or outdated comments.
Organize imports in three groups separated by blank lines (Go stdlib, 3rd-party excluding cloudposse/atmos, Atmos packages), sorted alphabetically. Maintain aliases: cfg, log, u, errUtils.
Add defer perf.Track(atmosConfig, "pkg.FuncName")() + blank line to all public functions. Use nil if no atmosConfig parameter.
All errors MUST be wrapped using static errors defined in errors/errors.go. Use errors.Join for combining multiple errors, fmt.Errorf with %w for adding string context, error builder for complex errors, and errors.Is() for error checking. NEVER use dynamic errors directly.
Use go.uber.org/mock/mockgen with //go:generate directives for mock generation. Never create manual mocks.
Use viper.BindEnv with ATMOS_ prefix for environment variables.
Use colors from pkg/ui/theme/colors.go for theme consistency.
Ensure Linux/macOS/Windows compatibility. Use SDKs over binaries. Use filepath.Join(), not hardcoded path separators.
Small focused files (<600 lines). One cmd/impl per file. Co-locate tests. Never use //revive:disable:file-length-limit.
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider us...
Files:
pkg/store/redis_store.gopkg/store/google_secret_manager_store_test.gopkg/store/google_secret_manager_store.gopkg/store/aws_ssm_param_store.gopkg/store/aws_ssm_param_store_test.gopkg/store/redis_store_test.gopkg/store/artifactory_store.gopkg/store/artifactory_store_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Prefer unit tests with mocks over integration tests. Use interfaces + dependency injection for testability. Generate mocks with go.uber.org/mock/mockgen. Use table-driven tests for comprehensive coverage. Target >80% code coverage.
Test behavior, not implementation. Never test stub functions. Avoid tautological tests. Make code testable with DI to avoid os.Exit and external systems. Use assert.ErrorIs(err, ErrSentinel) for stdlib/our errors.
**/*_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 fixtures
Files:
pkg/store/google_secret_manager_store_test.gopkg/store/aws_ssm_param_store_test.gopkg/store/redis_store_test.gopkg/store/artifactory_store_test.go
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:35:37.181Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.181Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/store/google_secret_manager_store_test.go
📚 Learning: 2025-11-24T17:35:20.287Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:35:20.287Z
Learning: Applies to **/*_test.go : Test behavior, not implementation. Never test stub functions. Avoid tautological tests. Make code testable with DI to avoid os.Exit and external systems. Use assert.ErrorIs(err, ErrSentinel) for stdlib/our errors.
Applied to files:
pkg/store/google_secret_manager_store_test.gopkg/store/artifactory_store_test.go
📚 Learning: 2025-11-24T17:35:20.287Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:35:20.287Z
Learning: Use interface-driven design with dependency injection for testability. Generate mocks with go.uber.org/mock/mockgen using //go:generate directives.
Applied to files:
pkg/store/aws_ssm_param_store_test.gopkg/store/redis_store_test.go
🧬 Code graph analysis (2)
pkg/store/google_secret_manager_store_test.go (1)
pkg/store/google_secret_manager_store.go (1)
GSMStore(34-40)
pkg/store/artifactory_store_test.go (1)
pkg/store/artifactory_store.go (1)
ArtifactoryStore(20-25)
⏰ 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). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/store/redis_store.go (1)
156-188: RedisStore.GetKey raw-key behavior looks correct.Using
keydirectly asredisKey(no prefix) plus the clarifying comments cleanly match the documented!store.getbehavior and the updated tests. No further issues here.pkg/store/aws_ssm_param_store.go (1)
257-308: SSMStore.GetKey raw-name behavior and docs are in sync.Using the provided
keydirectly (only normalizing a leading/) plus the clarifying comments is exactly what!store.getneeds; this keepsGet/Setprefix behavior unchanged while makingGetKeypredictable. Looks good.pkg/store/google_secret_manager_store.go (1)
290-327: GSMStore.GetKey docstring and behavior align.The updated comments correctly describe the raw-key behavior implemented here and line up with the new tests. No issues from this change.
pkg/store/aws_ssm_param_store_test.go (1)
709-895: SSM GetKey tests correctly assert no prefix is applied.Updating the mocked
GetParametercalls to use"/"+key(withprefixstill set on the store) is a good way to pin down the newGetKeysemantics and prevent regressions. The variety of JSON/non‑JSON cases still looks solid.
📝 WalkthroughWalkthroughA consistent refactor across store backends (Artifactory, AWS SSM, Google Secret Manager, Redis) removes automatic key prefixing from GetKey methods, allowing keys to be accessed as-is rather than transformed with store-level prefixes. Implementation and test files updated to reflect this behavioral change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (8)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-11-24T17:35:20.287ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.181ZApplied to files:
📚 Learning: 2025-11-24T17:35:20.287ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.181ZApplied to files:
🧬 Code graph analysis (2)pkg/store/google_secret_manager_store_test.go (1)
pkg/store/artifactory_store_test.go (1)
⏰ 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). (1)
🔇 Additional comments (9)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1814 +/- ##
==========================================
+ Coverage 72.63% 72.67% +0.04%
==========================================
Files 521 521
Lines 49598 49590 -8
==========================================
+ Hits 36025 36042 +17
+ Misses 10829 10801 -28
- Partials 2744 2747 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Windows tests are failing: |
|
@ohaibbq thanks for the PR, it looks good. use |
3 participants
what
All stores (besides Azure KeyVault) incorrectly added the store's prefix to calls to
GetKey. This updates them to match the expected behavior (according to the Atmos documentation)why
This updates the implementations to match the documentation. In the interim, we've configured multiple stores, one prefixed, one unprefixed in order to fetch arbitrary keys from GSM.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.