refactor(store): move backends into pkg/store/providers subpackage - #2575
Merged
Conversation
Move the concrete store backend implementations (AWS SSM, Azure Key Vault, Google Secret Manager, Redis, Artifactory) and the NewStoreRegistry factory into a new pkg/store/providers package, keeping pkg/store as a leaf that only exposes the Store interfaces, config/registry types, auth-config types, and error sentinels. The providers package imports pkg/store (never the reverse), mirroring the pkg/auth/providers and pkg/secrets/providers pattern. The shared unexported helpers used only by the moved code (getKey, parseOptions, and the errFormat constants) move with it, so pkg/store's public surface is unchanged. NewStoreRegistry is refactored into per-backend buildXStore helpers to keep cognitive complexity within limits. pkg/config now calls providers.NewStoreRegistry; pkg/schema continues to reference only the types in pkg/store. A depguard exclusion for pkg/store/providers is added so the cloud-SDK imports are permitted there, matching pkg/auth/providers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
June 5, 2026 23:46
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
no-release
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR replaces switch-based store construction with registered provider factories. It moves store implementations to ChangesStore provider refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/store/providers/redis_store.go (1)
65-68: ⚡ Quick winDouble-wrapping of
store.ErrParseRedisURLerror.
getRedisOptions(line 41) already wraps errors withstore.ErrParseRedisURL. Wrapping again here creates a redundant chain likeErrParseRedisURL: ErrParseRedisURL: <cause>.Consider returning the error directly since it's already properly wrapped:
Proposed fix
opts, err := getRedisOptions(&options) if err != nil { - return nil, fmt.Errorf(errFormat, store.ErrParseRedisURL, err) + return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/redis_store.go` around lines 65 - 68, The error returned from getRedisOptions is already wrapped with store.ErrParseRedisURL, so remove the redundant wrapping here: replace the fmt.Errorf(errFormat, store.ErrParseRedisURL, err) return with a plain return nil, err (i.e., return the err directly). Update the error handling around the call to getRedisOptions (the vars opts and err) so you propagate err unchanged instead of re-wrapping it.pkg/store/providers/azure_keyvault_store.go (1)
77-92: ⚡ Quick winVariable
storeshadows the imported package name.The local variable
store(line 77) shadows the importedstorepackage. While it works here since nostore.*references occur after this point in the function, it's a readability concern and could cause issues during future edits.Consider renaming to
sorkvStore:Proposed fix
- store := &AzureKeyVaultStore{ + s := &AzureKeyVaultStore{ vaultURL: options.VaultURL, prefix: prefix, stackDelimiter: &stackDelimiter, identityName: identityName, } // If no identity is configured, initialize the client eagerly (backward compatible behavior). if identityName == "" { - if err := store.initDefaultClient(); err != nil { + if err := s.initDefaultClient(); err != nil { return nil, err } } - return store, nil + return s, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/azure_keyvault_store.go` around lines 77 - 92, The local variable named `store` shadows the imported `store` package; rename the local variable (e.g., to `s` or `kvStore`) where `AzureKeyVaultStore` is instantiated so it no longer conflicts, update all references to that variable within the function (including the conditional that calls `initDefaultClient` and the final `return`), and ensure the type name `AzureKeyVaultStore` and method `initDefaultClient` remain unchanged.pkg/store/providers/google_secret_manager_store.go (1)
75-95: ⚡ Quick winVariable
storeshadows the imported package name.Same issue as Azure provider - the local variable
store(line 75) shadows the importedstorepackage, which can cause confusion.Consider renaming to
sorgsmStore:Proposed fix
- store := &GSMStore{ + s := &GSMStore{ projectID: options.ProjectID, credentials: options.Credentials, identityName: identityName, } if options.Prefix != nil { - store.prefix = *options.Prefix + s.prefix = *options.Prefix } if options.StackDelimiter != nil { - store.stackDelimiter = options.StackDelimiter + s.stackDelimiter = options.StackDelimiter } else { defaultDelimiter := "-" - store.stackDelimiter = &defaultDelimiter + s.stackDelimiter = &defaultDelimiter } - store.replication = createReplicationFromLocations(options.Locations) + s.replication = createReplicationFromLocations(options.Locations) - return store, nil + return s, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/google_secret_manager_store.go` around lines 75 - 95, The local variable named "store" shadows the imported "store" package; rename the local variable to something like "gsmStore" (or "s") where it's declared as &GSMStore{...}, update all subsequent uses in this function (assigning prefix, stackDelimiter, replication via createReplicationFromLocations(options.Locations) and the final "return store, nil") to use the new name, and return the renamed variable (e.g., "return gsmStore, nil") so the package name is not shadowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NOTICE`:
- Around line 835-839: The NOTICE file was manually edited resulting in
"Unknown" URLs (e.g., the go4.org/netipx entries) and CI failures; undo manual
changes and regenerate the NOTICE using the project's generator: run
./scripts/generate-notice.sh to produce an updated NOTICE, verify the generated
content replaces the manual edits (search for the go4.org/netipx entries to
confirm), then commit the generated NOTICE; do not hand-edit NOTICE in the
future.
In `@pkg/store/providers/registry.go`:
- Around line 71-75: The parseOptions error handling in buildAzureKeyVaultStore
(and the analogous GSM builder) currently returns ad-hoc fmt.Errorf strings;
update these to wrap the parse error with the package-level sentinel errors used
elsewhere (e.g., return nil, fmt.Errorf("%w: %v",
store.ErrParseAzureKeyVaultOptions, err) in buildAzureKeyVaultStore and
similarly use store.ErrParseGCPSecretManagerOptions (or the existing GSM
sentinel) in the GSM/GCP builder) so callers can reliably use errors.Is; locate
parseOptions calls in buildAzureKeyVaultStore and the GSM builder and replace
the direct fmt.Errorf messages with wrapping using the appropriate
store.ErrParse* sentinel.
---
Nitpick comments:
In `@pkg/store/providers/azure_keyvault_store.go`:
- Around line 77-92: The local variable named `store` shadows the imported
`store` package; rename the local variable (e.g., to `s` or `kvStore`) where
`AzureKeyVaultStore` is instantiated so it no longer conflicts, update all
references to that variable within the function (including the conditional that
calls `initDefaultClient` and the final `return`), and ensure the type name
`AzureKeyVaultStore` and method `initDefaultClient` remain unchanged.
In `@pkg/store/providers/google_secret_manager_store.go`:
- Around line 75-95: The local variable named "store" shadows the imported
"store" package; rename the local variable to something like "gsmStore" (or "s")
where it's declared as &GSMStore{...}, update all subsequent uses in this
function (assigning prefix, stackDelimiter, replication via
createReplicationFromLocations(options.Locations) and the final "return store,
nil") to use the new name, and return the renamed variable (e.g., "return
gsmStore, nil") so the package name is not shadowed.
In `@pkg/store/providers/redis_store.go`:
- Around line 65-68: The error returned from getRedisOptions is already wrapped
with store.ErrParseRedisURL, so remove the redundant wrapping here: replace the
fmt.Errorf(errFormat, store.ErrParseRedisURL, err) return with a plain return
nil, err (i.e., return the err directly). Update the error handling around the
call to getRedisOptions (the vars opts and err) so you propagate err unchanged
instead of re-wrapping it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3d8af25-6e61-4fef-915c-fcc924e9cd75
📒 Files selected for processing (28)
.golangci.ymlNOTICEinternal/exec/template_funcs_store_test.gointernal/exec/yaml_func_store_get_test.gointernal/exec/yaml_func_store_test.gopkg/config/utils.gopkg/store/config.gopkg/store/errors.gopkg/store/providers/artifactory_store.gopkg/store/providers/artifactory_store_noop_logger.gopkg/store/providers/artifactory_store_noop_logger_test.gopkg/store/providers/artifactory_store_test.gopkg/store/providers/aws_ssm_param_store.gopkg/store/providers/aws_ssm_param_store_test.gopkg/store/providers/azure_keyvault_store.gopkg/store/providers/azure_keyvault_store_test.gopkg/store/providers/google_secret_manager_store.gopkg/store/providers/google_secret_manager_store_test.gopkg/store/providers/identity_test.gopkg/store/providers/keys.gopkg/store/providers/mock_reliability_test.gopkg/store/providers/redis_store.gopkg/store/providers/redis_store_test.gopkg/store/providers/registry.gopkg/store/providers/registry_test.gopkg/store/registry.gopkg/store/store.gotests/store_artifactory_test.go
💤 Files with no reviewable changes (3)
- pkg/store/errors.go
- pkg/store/config.go
- pkg/store/store.go
Move NewStoreRegistry back into pkg/store and have each backend register its factory with the store package via store.Register from an init() function, replacing the type switch with a map lookup. pkg/store now owns both the StoreRegistry type and the build logic; pkg/store/providers holds only the concrete backends, each self-contained (its constructor plus a small factory adapter that parses options and warns on unsupported identity). pkg/config blank-imports pkg/store/providers so the built-in backends register at startup, mirroring the database/sql driver pattern. Behavior is unchanged: the same store types resolve, unknown types still return ErrStoreTypeNotFound, and the identity warnings are preserved. This addresses the confusion of having two registry.go files where the one under providers was named after the type it returns rather than what it does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/store/registry.go (1)
45-47: ⚡ Quick winWrap factory failures with the configured store name/type.
pkg/config/utils.gopropagates this error directly during bootstrap, so returning the raw provider error here makes multi-store misconfigurations harder to trace back to the failing entry.Suggested fix
s, err := factory(name, storeConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("build store %q (%s): %w", name, storeConfig.Type, err) }As per coding guidelines, user-facing errors should include clear troubleshooting context, and this path is part of config bootstrapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/registry.go` around lines 45 - 47, The factory call error returned from factory(name, storeConfig) should be wrapped with the configured store context so bootstrap errors show which store entry failed; change the error return to wrap err (e.g. using fmt.Errorf with %w) and include the store name and storeConfig.Type (or other identifying fields from storeConfig) in the message so callers get "store '<name>' (type='<type>') factory failed: <err>" context instead of the raw provider error.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/store/providers/google_secret_manager_store.go`:
- Around line 440-444: Add a new sentinel error ErrParseGSMOptions in
pkg/store/errors.go and modify buildGSMStore to wrap parseOptions failures with
that sentinel; specifically, in buildGSMStore (function name) replace the
current return for parseOptions error with something like return nil,
fmt.Errorf("%w: %v", store.ErrParseGSMOptions, err) so the error contract
matches other providers while still including the original parse error text.
In `@pkg/store/registry.go`:
- Around line 14-16: Add a sync.RWMutex to guard the global storeFactories map
and use it in Register and NewStoreRegistry: introduce a package-level var (e.g.
storeFactoriesMu sync.RWMutex), acquire storeFactoriesMu.Lock() when mutating
the map inside Register (and defer Unlock), and acquire storeFactoriesMu.RLock()
when reading the map inside NewStoreRegistry (and defer RUnlock) to prevent
concurrent map read/write panics; keep the rest of the Register and
NewStoreRegistry logic unchanged.
---
Nitpick comments:
In `@pkg/store/registry.go`:
- Around line 45-47: The factory call error returned from factory(name,
storeConfig) should be wrapped with the configured store context so bootstrap
errors show which store entry failed; change the error return to wrap err (e.g.
using fmt.Errorf with %w) and include the store name and storeConfig.Type (or
other identifying fields from storeConfig) in the message so callers get "store
'<name>' (type='<type>') factory failed: <err>" context instead of the raw
provider error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e7aba44-0a4c-4c2f-8b67-36123597339d
📒 Files selected for processing (10)
pkg/config/utils.gopkg/store/providers/artifactory_store.gopkg/store/providers/aws_ssm_param_store.gopkg/store/providers/azure_keyvault_store.gopkg/store/providers/google_secret_manager_store.gopkg/store/providers/providers.gopkg/store/providers/redis_store.gopkg/store/providers/registry_test.gopkg/store/registry.gopkg/store/store.go
💤 Files with no reviewable changes (1)
- pkg/store/store.go
✅ Files skipped from review due to trivial changes (1)
- pkg/store/providers/providers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/store/providers/registry_test.go
- pkg/store/providers/azure_keyvault_store.go
- pkg/store/providers/redis_store.go
…ry with mutex Address CodeRabbit review on the store providers refactor: - Wrap GSM and Azure Key Vault option-parse failures with new store.ErrParseGSMOptions / store.ErrParseAzureKeyVaultOptions sentinels so errors.Is works provider-agnostically, matching SSM/Artifactory/Redis. - Guard the global storeFactories map with sync.RWMutex; the exported Register can race with NewStoreRegistry and trip Go's concurrent map detector. - Add a -race concurrency test exercising Register vs NewStoreRegistry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/store/registry_test.go`:
- Around line 13-42: The test TestRegisterConcurrentWithNewStoreRegistry mutates
the global storeFactories via Register(...) but doesn't clean up, polluting
later tests; add a Reset() test helper in pkg/store/registry.go that acquires
storeFactoriesMu and reinitializes storeFactories (for tests only), then call
t.Cleanup(Reset) at the start of the test (or register Cleanup after creating
the goroutines) so registered types are cleared; if Reset already exists, simply
invoke t.Cleanup(Reset) in TestRegisterConcurrentWithNewStoreRegistry to ensure
isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b0c8448-b52e-44a0-831c-ec5cdf8d5711
📒 Files selected for processing (5)
pkg/store/errors.gopkg/store/providers/azure_keyvault_store.gopkg/store/providers/google_secret_manager_store.gopkg/store/registry.gopkg/store/registry_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/store/errors.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/store/registry.go
- pkg/store/providers/google_secret_manager_store.go
- pkg/store/providers/azure_keyvault_store.go
Add a Reset() helper to pkg/store/registry.go (test-only, mutex-guarded) that reinitializes storeFactories, mirroring the canonical registry pattern (cmd/internal/registry.go). Call t.Cleanup(Reset) in TestRegisterConcurrentWithNewStoreRegistry so its 50 throwaway types no longer leak into the global storeFactories map. Addresses CodeRabbit review on PR #2575. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2575 +/- ##
==========================================
+ Coverage 82.11% 82.19% +0.08%
==========================================
Files 1802 1802
Lines 175175 175214 +39
==========================================
+ Hits 143842 144020 +178
+ Misses 23565 23484 -81
+ Partials 7768 7710 -58
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The providers subpackage move re-triggered Codecov on the moved files as new code, dropping patch coverage to 61.57% (below the 80% target). Add targeted unit tests for the cheapest gaps using the existing mock infrastructure: - artifactory GetKey (0% -> 86%) via MockArtifactoryClient.DownloadFiles - redis getRedisOptions (50% -> 100%) and RedisClient (0% -> 100%) - gsm createSecret (45% -> 100%) status-code branches (AlreadyExists, NotFound, PermissionDenied, generic) Raises the providers package coverage to 85.1%. Addresses Codecov report on PR #2575. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/store/providers/artifactory_store_test.go (1)
3-17: ⚡ Quick winReorder imports to keep Atmos packages in the final group.
At Line 10-11, Atmos imports are mixed into the third-party section.
Proposed import block reorder
import ( "fmt" "os" "path/filepath" "testing" "github.com/aws/aws-sdk-go-v2/aws" - log "github.com/cloudposse/atmos/pkg/logger" - storepkg "github.com/cloudposse/atmos/pkg/store" "github.com/jfrog/jfrog-client-go/artifactory" "github.com/jfrog/jfrog-client-go/artifactory/services" al "github.com/jfrog/jfrog-client-go/utils/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + + log "github.com/cloudposse/atmos/pkg/logger" + storepkg "github.com/cloudposse/atmos/pkg/store" )As per coding guidelines, imports must be grouped as stdlib, then third-party, then Atmos packages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/artifactory_store_test.go` around lines 3 - 17, The import block mixes Atmos packages into the third-party group; move the Atmos imports (the imported aliases log and storepkg referencing github.com/cloudposse/atmos/pkg/logger and github.com/cloudposse/atmos/pkg/store) into the final import group so imports are ordered: stdlib first (fmt, os, filepath, testing), third-party next (aws, jfrog, testify, mock, al), and Atmos packages last; update the import section in pkg/store/providers/artifactory_store_test.go accordingly.Source: Coding guidelines
pkg/store/providers/redis_store_test.go (1)
3-14: ⚡ Quick winFix import group ordering at Line 10.
storepkg(Atmos import) is grouped before third-party imports. Keep stdlib → third-party → Atmos groups with blank lines.Proposed import block reorder
import ( "context" "encoding/json" "fmt" "testing" "time" - storepkg "github.com/cloudposse/atmos/pkg/store" "github.com/redis/go-redis/v9" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + + storepkg "github.com/cloudposse/atmos/pkg/store" )As per coding guidelines, “Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party, (3) Atmos packages.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/redis_store_test.go` around lines 3 - 14, Reorder the import block so it follows stdlib → third-party → Atmos grouping: move storepkg ("github.com/cloudposse/atmos/pkg/store") into the third group after the blank line following third-party imports (which include "github.com/redis/go-redis/v9", "github.com/stretchr/testify/assert", and "github.com/stretchr/testify/mock"), and ensure each group is alphabetized and separated by a blank line; update the import block that currently contains storepkg to match this ordering.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/store/providers/redis_store_test.go`:
- Around line 348-360: The env-fallback branch in getRedisOptions currently
returns raw redis.ParseURL errors which prevents errors.Is checks against the
sentinel ErrParseRedisURL from matching; update the ATMOS_REDIS_URL handling in
getRedisOptions (pkg/store/providers/redis_store.go) so that when
redis.ParseURL(envURL) returns an error you wrap it with the sentinel (e.g.
return nil, fmt.Errorf("%w: %v", storepkg.ErrParseRedisURL, err) or similar) and
return the wrapped error, and add a unit test in redis_store_test.go that sets
ATMOS_REDIS_URL to an invalid URL and asserts errors.Is(err,
storepkg.ErrParseRedisURL) to cover this branch.
---
Nitpick comments:
In `@pkg/store/providers/artifactory_store_test.go`:
- Around line 3-17: The import block mixes Atmos packages into the third-party
group; move the Atmos imports (the imported aliases log and storepkg referencing
github.com/cloudposse/atmos/pkg/logger and
github.com/cloudposse/atmos/pkg/store) into the final import group so imports
are ordered: stdlib first (fmt, os, filepath, testing), third-party next (aws,
jfrog, testify, mock, al), and Atmos packages last; update the import section in
pkg/store/providers/artifactory_store_test.go accordingly.
In `@pkg/store/providers/redis_store_test.go`:
- Around line 3-14: Reorder the import block so it follows stdlib → third-party
→ Atmos grouping: move storepkg ("github.com/cloudposse/atmos/pkg/store") into
the third group after the blank line following third-party imports (which
include "github.com/redis/go-redis/v9", "github.com/stretchr/testify/assert",
and "github.com/stretchr/testify/mock"), and ensure each group is alphabetized
and separated by a blank line; update the import block that currently contains
storepkg to match this ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 565d89ce-a44b-4484-b875-ce7055aa5f23
📒 Files selected for processing (3)
pkg/store/providers/artifactory_store_test.gopkg/store/providers/google_secret_manager_store_test.gopkg/store/providers/redis_store_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/store/providers/artifactory_store_test.go (1)
3-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReorganize imports into three groups per coding guidelines.
When adding the
storepkgimport, the import block should follow the three-group structure: stdlib, 3rd-party (NOT cloudposse/atmos), and Atmos packages, each separated by blank lines. Currently, Atmos imports (lines 10–11) are interleaved with 3rd-party imports.♻️ Proposed reorganization
import ( "fmt" "os" "path/filepath" "testing" "github.com/aws/aws-sdk-go-v2/aws" - log "github.com/cloudposse/atmos/pkg/logger" - storepkg "github.com/cloudposse/atmos/pkg/store" "github.com/jfrog/jfrog-client-go/artifactory" "github.com/jfrog/jfrog-client-go/artifactory/services" al "github.com/jfrog/jfrog-client-go/utils/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + + log "github.com/cloudposse/atmos/pkg/logger" + storepkg "github.com/cloudposse/atmos/pkg/store" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/store/providers/artifactory_store_test.go` around lines 3 - 17, Reorder the import block into three groups (stdlib, third-party, then Atmos packages) separated by blank lines: keep "fmt", "os", "path/filepath", "testing" in the stdlib group; move "github.com/aws/aws-sdk-go-v2/aws", "github.com/jfrog/jfrog-client-go/artifactory", "github.com/jfrog/jfrog-client-go/artifactory/services", "github.com/jfrog/jfrog-client-go/utils/log" (al), "github.com/stretchr/testify/assert", and "github.com/stretchr/testify/mock" into the third-party group; and place the Atmos imports "log \"github.com/cloudposse/atmos/pkg/logger\"" and "storepkg \"github.com/cloudposse/atmos/pkg/store\"" together in the final group, preserving aliases (log, storepkg, al) and existing import names.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/store/providers/artifactory_store_test.go`:
- Around line 3-17: Reorder the import block into three groups (stdlib,
third-party, then Atmos packages) separated by blank lines: keep "fmt", "os",
"path/filepath", "testing" in the stdlib group; move
"github.com/aws/aws-sdk-go-v2/aws",
"github.com/jfrog/jfrog-client-go/artifactory",
"github.com/jfrog/jfrog-client-go/artifactory/services",
"github.com/jfrog/jfrog-client-go/utils/log" (al),
"github.com/stretchr/testify/assert", and "github.com/stretchr/testify/mock"
into the third-party group; and place the Atmos imports "log
\"github.com/cloudposse/atmos/pkg/logger\"" and "storepkg
\"github.com/cloudposse/atmos/pkg/store\"" together in the final group,
preserving aliases (log, storepkg, al) and existing import names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6804c39-8ba7-4019-a6aa-3f4a6ac8d73c
📒 Files selected for processing (3)
pkg/store/providers/artifactory_store_test.gopkg/store/providers/google_secret_manager_store_test.gopkg/store/providers/redis_store_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/store/providers/redis_store_test.go
- pkg/store/providers/google_secret_manager_store_test.go
Raise patch coverage on the new pkg/store/providers subpackage and the store registry (Codecov reported 65.82% patch coverage on the moved files). Adds focused unit tests (no provider logic changes) reusing existing mockable client interfaces and helpers: provider validation/error branches, private helpers (getAccessKey env fallbacks, processDownloadedFile, assumeRole), factory parseOptions error paths, registry helpers (Register duplicate-panic, Reset, NewStoreRegistry unknown-type + factory-error), and noopLogger.Verbose. Providers package self-coverage 85.2% -> 94.4%; registry.go funcs now 100%. Also extract recurring string literals in cmd/terraform/utils.go into named constants to satisfy revive (add-constant), which the pre-commit hook flags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/terraform/utils.go (1)
704-704: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete the constant extraction pattern for stack.
Line 695 uses
logKeyComponentconstant, but this line still uses the hardcoded literal"stack". AddlogKeyStack = "stack"to the const block (lines 33-39) and use it here for consistency with the rest of this refactor.♻️ Suggested fix
Add to the const block:
const ( logKeyComponent = "component" logKeyError = "error" + logKeyStack = "stack" flagCI = "ci"Then update line 704:
stack, err := promptForStack(cmd, info.ComponentFromArg) - if err = handlePromptError(err, "stack"); err != nil { + if err = handlePromptError(err, logKeyStack); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/terraform/utils.go` at line 704, Add a new constant logKeyStack = "stack" to the existing const block alongside logKeyComponent, then replace the hardcoded literal "stack" in the handlePromptError call with the new logKeyStack constant (update the call at handlePromptError(err, "stack") to handlePromptError(err, logKeyStack)) so the file uses the same constant extraction pattern as logKeyComponent.
🧹 Nitpick comments (1)
cmd/terraform/utils.go (1)
34-35: ⚡ Quick winConsider centralizing log key constants if usage expands.
logKeyComponentand (the suggested)logKeyStackduplicate constants inpkg/ci/plugins/terraform/comments.go. For now the duplication is minimal, but if more log keys emerge or these are used in additional packages, centralizing them (perhaps inpkg/loggeror a newpkg/ci/constants) would prevent divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/terraform/utils.go` around lines 34 - 35, The two log key constants logKeyComponent and logKeyError (and the related logKeyStack found in pkg/ci/plugins/terraform/comments.go) should be centralized into a single shared constants location (e.g. add a new constants file in pkg/logger or pkg/ci/constants), export them (e.g. Component, Error, Stack) and replace the local definitions in cmd/terraform/utils.go and pkg/ci/plugins/terraform/comments.go to import and use the shared constants; update any references to the old local names to the new exported identifiers and run a quick build to ensure no missing imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/store/providers/aws_ssm_param_store_test.go`:
- Around line 994-1011: The test TestNewSSMStore_WithRolesAndDelimiter is
nondeterministic because it returns on NewSSMStore error, causing the test to
pass whether creation succeeds or fails; update the test so it deterministically
exercises field initialization: either guard the test with an environment
precondition (call tests.RequireAWSProfile or t.Skip when credentials are
absent) before calling NewSSMStore, or refactor to inject a mocked AWS SSM
client (use your mockgen-based mock and pass it into NewSSMStore or its
constructor path) and assert stackDelimiter/readRoleArn/writeRoleArn on the
resulting *SSMStore instead of returning on error. Ensure references to
NewSSMStore and SSMStore in the test are updated accordingly.
---
Outside diff comments:
In `@cmd/terraform/utils.go`:
- Line 704: Add a new constant logKeyStack = "stack" to the existing const block
alongside logKeyComponent, then replace the hardcoded literal "stack" in the
handlePromptError call with the new logKeyStack constant (update the call at
handlePromptError(err, "stack") to handlePromptError(err, logKeyStack)) so the
file uses the same constant extraction pattern as logKeyComponent.
---
Nitpick comments:
In `@cmd/terraform/utils.go`:
- Around line 34-35: The two log key constants logKeyComponent and logKeyError
(and the related logKeyStack found in pkg/ci/plugins/terraform/comments.go)
should be centralized into a single shared constants location (e.g. add a new
constants file in pkg/logger or pkg/ci/constants), export them (e.g. Component,
Error, Stack) and replace the local definitions in cmd/terraform/utils.go and
pkg/ci/plugins/terraform/comments.go to import and use the shared constants;
update any references to the old local names to the new exported identifiers and
run a quick build to ensure no missing imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8140e2d-ec54-4b4b-a5ea-30bf96ea173f
📒 Files selected for processing (9)
cmd/terraform/utils.gopkg/store/providers/artifactory_store_noop_logger_test.gopkg/store/providers/artifactory_store_test.gopkg/store/providers/aws_ssm_param_store_test.gopkg/store/providers/azure_keyvault_store_test.gopkg/store/providers/google_secret_manager_store_test.gopkg/store/providers/redis_store.gopkg/store/providers/redis_store_test.gopkg/store/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/store/providers/artifactory_store_noop_logger_test.go
- pkg/store/providers/redis_store.go
…ers-subpackage # Conflicts: # cmd/terraform/utils.go
The Acceptance Tests (linux) job failed at the Codecov upload step, not in
any test — all Go tests passed. The codecov-action could not verify the GPG
signature of its downloaded CLI:
gpg: Can't check signature: No public key
==> Could not verify signature. Please contact Codecov if problem continues
Exiting...
##[error]Process completed with exit code 1.
Codecov's GPG keyserver is intermittently unreachable, so the integrity check
fails and, with fail_ci_if_error: true, takes the whole job down despite a
green test suite. Set skip_validation: true to bypass the GPG integrity check
(the CLI is still fetched over HTTPS from cli.codecov.io). fail_ci_if_error
stays true so genuine upload failures still surface.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…ers-subpackage # Conflicts: # pkg/cache/filelock_unix.go
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…ers-subpackage # Conflicts: # pkg/store/providers/aws_ssm_param_store.go
Merging main pulled in struct/doc-comment changes that left the embedded atmos.yaml JSON Schema out of sync with the Go config structs. Regenerated via `go generate ./pkg/config/schema` per TestEmbeddedSchemaIsCurrent.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 22:41 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
The Validation (affected) job's editorconfig check runs against files touched by this branch, and both scripts used space indentation while .editorconfig declares `indent_style = tab` for *.sh. Convert their code indentation to tabs to match; the one heredoc with literal 2-space-indented output content (the NOTICE footer template) is wrapped in editorconfig-checker-disable/-enable so its generated output stays byte-identical. Editing .editorconfig itself was considered instead, but it triggers `atmos validate editorconfig --affected` to fall back to validating every tracked file (by design, since a rule change can affect any file), which surfaces thousands of pre-existing violations unrelated to this branch.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 23:59 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Adds logKeyStack alongside the existing logKeyComponent and replaces hardcoded "stack" log-key/prompt-name literals with it.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 31, 2026 20:50 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/test.yml (2)
300-309: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore Playwright download coverage in CI.
TestPlaywrightDriverDownload_IntegrationandTestPlaywrightDriverDownload_WithSAML2AWSskip unlessRUN_PLAYWRIGHT_INTEGRATION=1, and no workflow sets this variable. Re-add it with a short comment if the heavy Playwright download integration should run in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 300 - 309, Update the Linux acceptance-test step identified by “Acceptance tests with coverage” to set RUN_PLAYWRIGHT_INTEGRATION=1 in its env block, alongside the existing CI variables, and add a brief comment documenting that it enables Playwright download integration coverage.
1024-1027: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd
cache: falseto thissetup-gostep for consistency.Every other
actions/setup-goinvocation in this file (lines 100-106, 257-264, 519-522, 696-698) setscache: falseto avoid restoring or saving mutable cache entries from PR-controlled keys. This step in thecontainer-stepjob omits it, so it keepssetup-go's default caching behavior enabled and reintroduces the same cache-poisoning exposure the rest of the file avoids.🔒 Proposed fix to align this step with the rest of the file
- name: Set up Go for the Buildx cache integration test uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: go-version-file: "go.mod" + cache: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 1024 - 1027, Update the “Set up Go for the Buildx cache integration test” setup-go step to set cache: false, matching the other actions/setup-go invocations in the workflow while preserving its existing Go version configuration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 300-309: Update the Linux acceptance-test step identified by
“Acceptance tests with coverage” to set RUN_PLAYWRIGHT_INTEGRATION=1 in its env
block, alongside the existing CI variables, and add a brief comment documenting
that it enables Playwright download integration coverage.
- Around line 1024-1027: Update the “Set up Go for the Buildx cache integration
test” setup-go step to set cache: false, matching the other actions/setup-go
invocations in the workflow while preserving its existing Go version
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c3cfc1b-5db3-40c7-af2a-ab00c9de2f82
📒 Files selected for processing (2)
.github/workflows/test.yml.golangci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .golangci.yml
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…ers-subpackage # Conflicts: # scripts/generate-notice.sh
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
August 2, 2026 02:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Andriy Knysh (aknysh)
temporarily deployed
to
screengrabs
August 2, 2026 13:33 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Andriy Knysh (aknysh)
approved these changes
Aug 2, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
pkg/storeinto a newpkg/store/providerssubpackage, keepingpkg/storeas a pure interface/type boundary (theStoreinterfaces,StoreConfig/StoresConfig/StoreRegistrytypes, auth-config types, error sentinels, and generated mocks).pkg/storeownsStoreRegistry,NewStoreRegistry, and aRegister(type, factory)API; each backend registers its factory from aninit(), so the typeswitchis replaced by a map lookup.pkg/configblank-importspkg/store/providersso the built-in backends register at startup (database/sql driver pattern).pkg/store/providersexclusion to theprovider-agnostic-authdepguard rule so its cloud-SDK imports are permitted (matchingpkg/auth/providers).why
pkg/auth/providers/pkg/secrets/providerslayout and makingpkg/storea clean type/interface package.providersbut was named after theStoreRegistrytype it returned;pkg/storenow owns both the registry type and its construction, and adding a backend is one self-contained file that registers itself.ErrStoreTypeNotFound, identity warnings are preserved. Builds clean, all affected tests pass, andgolangci-lint --new-from-rev=origin/mainreports zero issues.references
Summary by CodeRabbit