Repository navigation
refactor(auth): move ECR login from auth to aws namespace (ATMOS-37) - #2144
Merged
Andriy Knysh (aknysh) merged 8 commits intoMar 31, 2026
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2144 +/- ##
==========================================
+ Coverage 77.16% 77.20% +0.03%
==========================================
Files 1034 1034
Lines 97530 97542 +12
==========================================
+ Hits 75259 75303 +44
+ Misses 18068 18035 -33
- Partials 4203 4204 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ATMOS-37) - Move `atmos auth ecr-login` command to `atmos aws ecr login` under the AWS namespace - Create new `cmd/aws/ecr/` package with parent command and login subcommand - Add `--identity` flag locally (no longer inherited from auth namespace) - Move ECR login tests to new package - Move and update documentation from auth/ to aws/ namespace - Update all cross-references in tutorials, blog posts, and PDRs - Update interface comments in pkg/auth/types/interfaces.go Rationale: The `auth` namespace must remain provider-agnostic. AWS-specific commands like ECR login belong under the `atmos aws` namespace hierarchy, following the pattern established by `atmos aws eks update-kubeconfig`. Closes ATMOS-37 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The auth invalid-command snapshot listed ecr-login as a valid subcommand. Since ecr-login has been moved to the aws namespace, it no longer appears in the auth subcommand list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ben (Benbentwo)
force-pushed
the
feature/atmos-37-refactor-atmos-auth-ecr-login-to-remove-aws-specific
branch
from
March 23, 2026 15:21
4226e07 to
9e6d3da
Compare
The auth configuration page still referenced /cli/commands/auth/ecr-login which was moved to /cli/commands/aws/ecr-login in the namespace refactor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ben (Benbentwo)
marked this pull request as ready for review
March 30, 2026 16:42
…in-to-remove-aws-specific
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ac031059
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Contributor
📝 WalkthroughWalkthroughECR login was moved from the auth command tree ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User"
participant CLI as "atmos CLI\n(aws ecr login)"
participant Cmd as "loginCmd.RunE"
participant Builder as "createAuthManager"
participant Auth as "AuthManager"
participant Perf as "perf.Track"
User->>CLI: invoke "atmos aws ecr login [integration] --identity ... --registry ..."
CLI->>Cmd: dispatch to login command
Cmd->>Perf: track start (aws.ecr.executeLoginCommand)
alt explicit registry(s)
Cmd->>Perf: track start (aws.ecr.executeExplicitRegistries)
Cmd->>Auth: execute explicit registry login flow
else integration / identity
Cmd->>Builder: createAuthManager(config, cliConfigPath)
Builder-->>Cmd: AuthManager instance / error
Cmd->>Auth: ExecuteIntegration or ExecuteIdentityIntegrations
end
Auth-->>Cmd: results / errors
Cmd-->>User: exit status / output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/aws/ecr/login.go (1)
48-60:⚠️ Potential issue | 🟠 MajorUse
GetIdentityFromFlags()to read the identity flag.Lines 58-60 read the identity flag directly via
GetString(), which bypasses the standard identity parsing path. This skips environment variable fallback (ATMOS_IDENTITY) and the interactive selection prompt that should trigger when the user passes--identitywithout a value. UseGetIdentityFromFlags(cmd, os.Args)instead, as done elsewhere in the codebase (e.g.,cmd/describe_component.go).Also applies to: 149-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login.go` around lines 48 - 60, Replace direct flag reading of the identity in executeLoginCommand with the shared helper so identity parsing, env fallback and interactive prompts are respected: call GetIdentityFromFlags(cmd, os.Args) instead of cmd.Flags().GetString("identity") and use its returned identity value where identityName is used; do the same replacement for the similar code block around lines 149-158 to ensure consistent behavior across the file (refer to executeLoginCommand and GetIdentityFromFlags for locating the code).
🧹 Nitpick comments (1)
cmd/aws/ecr/login_test.go (1)
68-71: Make the no-args test exercise behavior, not the sentinel declaration.This only proves
errUtils.ErrECRLoginNoArgsexists. It would still pass ifexecuteLoginCommandnever returned that error. Please assert the real no-args path witherrors.Is(err, errUtils.ErrECRLoginNoArgs)once the command logic is injectable/testable.As per coding guidelines, "Test behavior, not implementation. Never test stub functions. Avoid tautological tests.".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 68 - 71, The test currently only asserts the sentinel errUtils.ErrECRLoginNoArgs exists; change TestLoginCmd_NoArgsError to exercise the no-args behavior by invoking the command logic (e.g., call executeLoginCommand with an empty args slice or run the login command entrypoint with no arguments) and capture its returned error, then assert errors.Is(err, errUtils.ErrECRLoginNoArgs). Ensure the test imports the standard errors package and uses the actual command function (executeLoginCommand or the login command's Execute method) so the assertion verifies behavior not just the sentinel declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/ecr-authentication.md`:
- Line 906: Update the stale checklist entry in docs/prd/ecr-authentication.md:
replace the test file path text "cmd/auth_ecr_login_test.go" with the new
location "cmd/aws/ecr/login_test.go" so the checkbox line reads that the unit
tests for the atmos aws ecr login command are in cmd/aws/ecr/login_test.go;
ensure only the path string is changed and formatting of the checklist line (the
"[x]" and backticks) is preserved.
In `@website/docs/cli/commands/aws/ecr-login.mdx`:
- Around line 159-162: The example uses an undefined integration name "dev/ecr";
update the example command to use one of the integration names declared earlier
(e.g., "dev/ecr/primary" or "dev/ecr/secondary") so it matches the config
section. Specifically, change the atmos command in the example (the line with
"atmos aws ecr login dev/ecr") to use "dev/ecr/primary" (or "dev/ecr/secondary")
to avoid copy/paste mismatch and ensure the example works as written.
---
Outside diff comments:
In `@cmd/aws/ecr/login.go`:
- Around line 48-60: Replace direct flag reading of the identity in
executeLoginCommand with the shared helper so identity parsing, env fallback and
interactive prompts are respected: call GetIdentityFromFlags(cmd, os.Args)
instead of cmd.Flags().GetString("identity") and use its returned identity value
where identityName is used; do the same replacement for the similar code block
around lines 149-158 to ensure consistent behavior across the file (refer to
executeLoginCommand and GetIdentityFromFlags for locating the code).
---
Nitpick comments:
In `@cmd/aws/ecr/login_test.go`:
- Around line 68-71: The test currently only asserts the sentinel
errUtils.ErrECRLoginNoArgs exists; change TestLoginCmd_NoArgsError to exercise
the no-args behavior by invoking the command logic (e.g., call
executeLoginCommand with an empty args slice or run the login command entrypoint
with no arguments) and capture its returned error, then assert errors.Is(err,
errUtils.ErrECRLoginNoArgs). Ensure the test imports the standard errors package
and uses the actual command function (executeLoginCommand or the login command's
Execute method) so the assertion verifies behavior not just the sentinel
declaration.
🪄 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: d943dbb4-5747-4fb4-acdd-16d43eb55487
📒 Files selected for processing (13)
cmd/auth_ecr_login_test.gocmd/aws/aws.gocmd/aws/ecr/ecr.gocmd/aws/ecr/login.gocmd/aws/ecr/login_test.godocs/prd/ecr-authentication.mdpkg/auth/types/interfaces.gotests/snapshots/TestCLICommands_atmos_auth_invalid-command.stderr.goldenwebsite/blog/2025-12-15-ecr-authentication-integration.mdxwebsite/docs/cli/commands/auth/auth-login.mdxwebsite/docs/cli/commands/aws/ecr-login.mdxwebsite/docs/cli/configuration/auth/index.mdxwebsite/docs/tutorials/ecr-authentication.mdx
💤 Files with no reviewable changes (2)
- tests/snapshots/TestCLICommands_atmos_auth_invalid-command.stderr.golden
- cmd/auth_ecr_login_test.go
- Restore positional "help" arg handling dropped during refactor - Fix stale test path in PRD (cmd/auth_ecr_login_test.go -> cmd/aws/ecr/login_test.go) - Fix integration name mismatch in docs (dev/ecr -> dev/ecr/primary) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
cmd/aws/ecr/login.go (1)
88-92:⚠️ Potential issue | 🔴 CriticalHandle interactive identity selection before calling ExecuteIdentityIntegrations.
When
--identityis used without a value,identityNamebecomescfg.IdentityFlagSelectValue("SELECT"). The code passes this directly toExecuteIdentityIntegrationsat line 91, which expects a real identity name and will fail with an error about identity "SELECT" not found.Before calling
ExecuteIdentityIntegrations, check ifidentityName == cfg.IdentityFlagSelectValueand either trigger interactive selection (similar tocmd/terraform/utils.go:361-363orcmd/devcontainer/start.go:60-77) or show a user-friendly error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login.go` around lines 88 - 92, Check for the marker value cfg.IdentityFlagSelectValue before calling ExecuteIdentityIntegrations: if identityName == cfg.IdentityFlagSelectValue, prompt the user to interactively select an identity (reuse the interactive selection flow used in cmd/terraform/utils.go or cmd/devcontainer/start.go) and assign the chosen name to identityName, or return a clear user-facing error indicating that an identity must be selected; only call authManager.ExecuteIdentityIntegrations(ctx, identityName) when identityName is a real identity string.
🧹 Nitpick comments (5)
cmd/aws/ecr/login_test.go (4)
73-85: Duplicate ofTestCreateAuthManager_Success.Both tests use identical config with empty maps. Consider consolidating or making this test meaningfully different (e.g., test with zero-value
Realmor other edge case).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 73 - 85, TestCreateAuthManager_EmptyConfig is a duplicate of TestCreateAuthManager_Success; either delete it or change it to cover a distinct edge case. Update TestCreateAuthManager_EmptyConfig (or remove the function) so it is not identical: e.g., set Realm = "" (zero-value) or alter Providers/Identities/Integrations to nil instead of empty maps and assert the expected behavior (error or nil manager) when calling createAuthManager; ensure references to createAuthManager and TestCreateAuthManager_Success remain consistent.
68-71: Test only verifies sentinel existence.This test doesn't validate behavior—it just confirms the error variable isn't nil. Consider testing that
executeLoginCommandactually returns this error when no args/flags are provided (though that requires mocking config initialization).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 68 - 71, The test currently only checks the sentinel errUtils.ErrECRLoginNoArgs exists; change it to call executeLoginCommand (the command entry point) with no args/flags and assert the returned error equals errUtils.ErrECRLoginNoArgs. If executeLoginCommand depends on config initialization, mock or stub the config initialization used by executeLoginCommand (or provide a minimal test config) so the call runs deterministically, then use assert.ErrorIs/assert.Equal to verify the exact sentinel is returned.
1-11: MissingTestMainwith subprocess gate.Per coding guidelines, test files should include a
TestMainwith_ATMOS_TEST_EXIT_ONEenvironment gate for cross-platform subprocess testing.♻️ Add TestMain
package ecr import ( + "os" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/schema" ) +func TestMain(m *testing.M) { + // Cross-platform subprocess exit helper. + if os.Getenv("_ATMOS_TEST_EXIT_ONE") == "1" { + os.Exit(1) + } + os.Exit(m.Run()) +} + func TestCreateAuthManager_Success(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 1 - 11, Add a TestMain in this package's test file to implement the subprocess gate used by cross-platform tests: create a TestMain(m *testing.M) that checks the _ATMOS_TEST_EXIT_ONE environment variable and if set runs os.Exit(1) (or otherwise executes the gate behavior) before calling m.Run(), ensuring subprocess-based tests in package ecr obey the exit-one gate; place this TestMain in cmd/aws/ecr/login_test.go alongside existing imports and use testing.M and os packages to implement the gate.
145-151: Redundant nil check after require.Line 148's
if loginCmd.Parent() != nilis unnecessary—ifParent()returned nil, line 147 would have already failed the test.♻️ Simplify
func TestLoginCmd_ParentIsEcrCmd(t *testing.T) { // Verify that login is a child of ecr command. - assert.NotNil(t, loginCmd.Parent()) - if loginCmd.Parent() != nil { - assert.Equal(t, "ecr", loginCmd.Parent().Name()) - } + require.NotNil(t, loginCmd.Parent()) + assert.Equal(t, "ecr", loginCmd.Parent().Name()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 145 - 151, Replace the redundant nil-check by using require to fail fast: in TestLoginCmd_ParentIsEcrCmd call require.NotNil(t, loginCmd.Parent()) (instead of assert.NotNil) and then directly assert.Equal(t, "ecr", loginCmd.Parent().Name()); this keeps the fast-fail semantics and removes the unnecessary if block around loginCmd.Parent().cmd/aws/ecr/login.go (1)
122-125: Consider using a static error sentinel.The error message uses a dynamic format string. Per coding guidelines, errors should be wrapped using static errors from
errors/errors.go.♻️ Suggested approach
If a sentinel like
ErrECRLoginFailedexists or can be added:- return fmt.Errorf("ECR login failed for %s: %w", registry, err) + return fmt.Errorf("%w for %s: %w", errUtils.ErrECRLoginFailed, registry, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login.go` around lines 122 - 125, The error returned from awsCloud.GetAuthorizationToken is wrapped with a dynamic message via fmt.Errorf; change this to wrap a static sentinel error (add ErrECRLoginFailed in errors/errors.go if it doesn't exist) and wrap the original error with that sentinel while preserving context (e.g., include the registry) using fmt.Errorf with %w; update the return in the GetAuthorizationToken error branch (the block handling result, err from awsCloud.GetAuthorizationToken) to return the sentinel-wrapped error instead of the current dynamic fmt.Errorf message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/aws/ecr/login.go`:
- Around line 88-92: Check for the marker value cfg.IdentityFlagSelectValue
before calling ExecuteIdentityIntegrations: if identityName ==
cfg.IdentityFlagSelectValue, prompt the user to interactively select an identity
(reuse the interactive selection flow used in cmd/terraform/utils.go or
cmd/devcontainer/start.go) and assign the chosen name to identityName, or return
a clear user-facing error indicating that an identity must be selected; only
call authManager.ExecuteIdentityIntegrations(ctx, identityName) when
identityName is a real identity string.
---
Nitpick comments:
In `@cmd/aws/ecr/login_test.go`:
- Around line 73-85: TestCreateAuthManager_EmptyConfig is a duplicate of
TestCreateAuthManager_Success; either delete it or change it to cover a distinct
edge case. Update TestCreateAuthManager_EmptyConfig (or remove the function) so
it is not identical: e.g., set Realm = "" (zero-value) or alter
Providers/Identities/Integrations to nil instead of empty maps and assert the
expected behavior (error or nil manager) when calling createAuthManager; ensure
references to createAuthManager and TestCreateAuthManager_Success remain
consistent.
- Around line 68-71: The test currently only checks the sentinel
errUtils.ErrECRLoginNoArgs exists; change it to call executeLoginCommand (the
command entry point) with no args/flags and assert the returned error equals
errUtils.ErrECRLoginNoArgs. If executeLoginCommand depends on config
initialization, mock or stub the config initialization used by
executeLoginCommand (or provide a minimal test config) so the call runs
deterministically, then use assert.ErrorIs/assert.Equal to verify the exact
sentinel is returned.
- Around line 1-11: Add a TestMain in this package's test file to implement the
subprocess gate used by cross-platform tests: create a TestMain(m *testing.M)
that checks the _ATMOS_TEST_EXIT_ONE environment variable and if set runs
os.Exit(1) (or otherwise executes the gate behavior) before calling m.Run(),
ensuring subprocess-based tests in package ecr obey the exit-one gate; place
this TestMain in cmd/aws/ecr/login_test.go alongside existing imports and use
testing.M and os packages to implement the gate.
- Around line 145-151: Replace the redundant nil-check by using require to fail
fast: in TestLoginCmd_ParentIsEcrCmd call require.NotNil(t, loginCmd.Parent())
(instead of assert.NotNil) and then directly assert.Equal(t, "ecr",
loginCmd.Parent().Name()); this keeps the fast-fail semantics and removes the
unnecessary if block around loginCmd.Parent().
In `@cmd/aws/ecr/login.go`:
- Around line 122-125: The error returned from awsCloud.GetAuthorizationToken is
wrapped with a dynamic message via fmt.Errorf; change this to wrap a static
sentinel error (add ErrECRLoginFailed in errors/errors.go if it doesn't exist)
and wrap the original error with that sentinel while preserving context (e.g.,
include the registry) using fmt.Errorf with %w; update the return in the
GetAuthorizationToken error branch (the block handling result, err from
awsCloud.GetAuthorizationToken) to return the sentinel-wrapped error instead of
the current dynamic fmt.Errorf message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15db3e0b-c509-482a-b281-673b2f0cb6b4
📒 Files selected for processing (4)
cmd/aws/ecr/login.gocmd/aws/ecr/login_test.godocs/prd/ecr-authentication.mdwebsite/docs/cli/commands/aws/ecr-login.mdx
✅ Files skipped from review due to trivial changes (2)
- website/docs/cli/commands/aws/ecr-login.mdx
- docs/prd/ecr-authentication.md
…in-to-remove-aws-specific
Critical: - Handle __SELECT__ identity value — return ErrECRIdentitySelect instead of passing sentinel to ExecuteIdentityIntegrations - Remove forbidden os.Getenv call (ATMOS_IDENTITY handled by global flag system) Code quality: - Extract executeWithAuthManager to reduce cyclomatic complexity - Replace dynamic "ECR login failed" error with static ErrECRLoginFailed sentinel - Fix redundant nil check in TestLoginCmd_ParentIsEcrCmd (use require.NotNil) - Differentiate TestCreateAuthManager_EmptyConfig (nil vs empty maps) - Improve TestLoginCmd_NoArgsError comment Test coverage: - Add TestExecuteWithAuthManager_NoArgs and _SelectSentinel - Coverage: 21.8% → 34.5% New sentinel errors: ErrECRLoginFailed, ErrECRIdentitySelect Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (2)
cmd/aws/ecr/login.go (2)
54-79:⚠️ Potential issue | 🟠 MajorDon't initialize Atmos config before the
--registryfast path.The explicit-registry mode only needs ambient AWS creds plus Docker config, but
cfg.InitCliConfig()runs first. That meansatmos aws ecr login --registry ...can fail on unrelated Atmos config issues before the explicit path is even reached.Suggested fix.
func executeLoginCommand(cmd *cobra.Command, args []string) error { // Handle positional "help" argument (e.g., "atmos aws ecr login help"). if len(args) > 0 && args[0] == "help" { return cmd.Help() } - // Load atmos config. - atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) - if err != nil { - return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrFailedToInitConfig, err) - } - defer perf.Track(&atmosConfig, "aws.ecr.executeLoginCommand")() - ctx := context.Background() // Get flag values (errors are ignored as flags are guaranteed to exist by Cobra). identityName, _ := cmd.Flags().GetString("identity") registries, _ := cmd.Flags().GetStringArray("registry") @@ // Case 1: Explicit registries (uses current AWS credentials from environment). if len(registries) > 0 { return executeExplicitRegistries(ctx, registries) } + // Load Atmos config only for auth-manager modes. + atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) + if err != nil { + return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrFailedToInitConfig, err) + } + defer perf.Track(&atmosConfig, "aws.ecr.executeLoginCommand")() + // Cases 2 & 3 require auth manager. return executeWithAuthManager(ctx, &atmosConfig, identityName, integrationName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login.go` around lines 54 - 79, The InitCliConfig call and its perf.Track defer should be delayed until after the explicit-registry fast path so that --registry uses only ambient AWS creds; specifically, remove or move cfg.InitCliConfig(...) and defer perf.Track(&atmosConfig, ...) out of the top of aws/ecr/login.go and only call them just before invoking executeWithAuthManager(...). Keep the existing fast-path check that reads flags and calls executeExplicitRegistries(ctx, registries) unchanged so that when len(registries) > 0 the function returns without initializing atmosConfig.
89-100:⚠️ Potential issue | 🟠 MajorReject
integration+--identityinstead of ignoring one input.When both are set, the positional integration path wins and
--identityis dropped on the floor. In this command, that is too easy to misread and can log into the wrong target.Suggested fix.
func executeWithAuthManager(ctx context.Context, atmosConfig *schema.AtmosConfiguration, identityName, integrationName string) error { authManager, err := createAuthManager(&atmosConfig.Auth, atmosConfig.CliConfigPath) if err != nil { return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrFailedToInitializeAuthManager, err) } + if integrationName != "" && identityName != "" { + return fmt.Errorf("%w: --identity cannot be combined with an integration argument", errUtils.ErrMutuallyExclusiveFlags) + } + // Case 2: Named integration. if integrationName != "" { return authManager.ExecuteIntegration(ctx, integrationName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login.go` around lines 89 - 100, Add an explicit rejection when both integrationName and identityName are provided: before the existing branches, check if integrationName != "" && identityName != "" and return a new descriptive error (e.g., errUtils.ErrECRIntegrationAndIdentity) instead of letting integrationName win; keep the rest of the logic using authManager.ExecuteIntegration(ctx, integrationName) and authManager.ExecuteIdentityIntegrations(ctx, identityName) unchanged, and add the new error constant to errUtils with an appropriate message for the CLI user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/aws/ecr/login.go`:
- Around line 164-172: The help text for the --identity flag on loginCmd
promises an interactive selection but the flag is wired to
cfg.IdentityFlagSelectValue and executeWithAuthManager rejects that sentinel
with ErrECRIdentitySelect; update behavior so the help matches reality by either
removing the "Use without value to interactively select." wording from
loginCmd.Flags().StringP(...) or implement the interactive selector path inside
executeWithAuthManager (or the auth flow it calls) to handle
cfg.IdentityFlagSelectValue and avoid returning ErrECRIdentitySelect. Reference
loginCmd, the "identity" flag Lookup and NoOptDefVal assignment,
executeWithAuthManager, ErrECRIdentitySelect, and cfg.IdentityFlagSelectValue
when making the change.
---
Outside diff comments:
In `@cmd/aws/ecr/login.go`:
- Around line 54-79: The InitCliConfig call and its perf.Track defer should be
delayed until after the explicit-registry fast path so that --registry uses only
ambient AWS creds; specifically, remove or move cfg.InitCliConfig(...) and defer
perf.Track(&atmosConfig, ...) out of the top of aws/ecr/login.go and only call
them just before invoking executeWithAuthManager(...). Keep the existing
fast-path check that reads flags and calls executeExplicitRegistries(ctx,
registries) unchanged so that when len(registries) > 0 the function returns
without initializing atmosConfig.
- Around line 89-100: Add an explicit rejection when both integrationName and
identityName are provided: before the existing branches, check if
integrationName != "" && identityName != "" and return a new descriptive error
(e.g., errUtils.ErrECRIntegrationAndIdentity) instead of letting integrationName
win; keep the rest of the logic using authManager.ExecuteIntegration(ctx,
integrationName) and authManager.ExecuteIdentityIntegrations(ctx, identityName)
unchanged, and add the new error constant to errUtils with an appropriate
message for the CLI user.
🪄 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: d647ffab-c500-4e39-a167-471af587608d
📒 Files selected for processing (3)
cmd/aws/ecr/login.gocmd/aws/ecr/login_test.goerrors/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/aws/ecr/login_test.go
…lags - Move cfg.InitCliConfig after --registry check so explicit registry mode doesn't require a valid atmos.yaml - Reject ambiguous input when both integration name and --identity are provided (ErrMutuallyExclusiveFlags) - Fix --identity help text (remove unsupported interactive selection promise) - Add TestExecuteWithAuthManager_MutuallyExclusiveFlags - Coverage: 34.5% → 36.8% Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/aws/ecr/login_test.go (1)
70-74: Sentinel existence check is redundant.
TestExecuteWithAuthManager_NoArgs(line 173) already tests the actual behavior. This test just confirms a package-level var isn't nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws/ecr/login_test.go` around lines 70 - 74, The test TestLoginCmd_NoArgsError is redundant because TestExecuteWithAuthManager_NoArgs already covers the behavior; remove the TestLoginCmd_NoArgsError function and any direct sentinel-only assertions (errUtils.ErrECRLoginNoArgs) from the file so tests only exercise the real behavior via TestExecuteWithAuthManager_NoArgs, ensuring no other tests rely on that sentinel-only check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/aws/ecr/login_test.go`:
- Around line 70-74: The test TestLoginCmd_NoArgsError is redundant because
TestExecuteWithAuthManager_NoArgs already covers the behavior; remove the
TestLoginCmd_NoArgsError function and any direct sentinel-only assertions
(errUtils.ErrECRLoginNoArgs) from the file so tests only exercise the real
behavior via TestExecuteWithAuthManager_NoArgs, ensuring no other tests rely on
that sentinel-only check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57c737f3-ae6f-4324-9c24-39c9c191adf0
📒 Files selected for processing (2)
cmd/aws/ecr/login.gocmd/aws/ecr/login_test.go
Andriy Knysh (aknysh)
approved these changes
Mar 31, 2026
Andriy Knysh (aknysh)
deleted the
feature/atmos-37-refactor-atmos-auth-ecr-login-to-remove-aws-specific
branch
March 31, 2026 13:58
|
These changes were released in v1.214.0-test.1. |
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
May 30, 2026
Resolve conflicts arising from main's PR #2144 (refactor(auth): move ECR login from auth to aws namespace), which renamed `atmos auth ecr-login` -> `atmos aws ecr login` and relocated its docs from cli/commands/auth/ to cli/commands/aws/. - errors/errors.go: keep both the ECR Public and EKS error blocks. - pkg/auth/integrations/types.go: keep KindAWSECRPublic alongside the now- implemented KindAWSEKS (dropped the stale "Future" comment). - ecr-login.mdx / ecr-authentication.mdx / blog: align all ECR Public examples to the canonical `atmos aws ecr login` command name. - roadmap.js: recompute auth initiative progress to 93 (28 shipped / 30 total after the merge adds the ECR Public registry authentication milestone). Adapt the branch's ECR Public integration to main's evolved interfaces: - Use the now-exported BuildAWSConfigFromCreds helper. - Implement Cleanup() and Environment() on ECRPublicIntegration, mirroring the private ECRIntegration (interface gained these methods in main). - Add errWrapFmt constant for the repeated "%w: %w" wrap format (revive). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
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
atmos auth ecr-logincommand toatmos aws ecr loginunder the AWS namespacecmd/aws/ecr/package with parent ECR command and login subcommandauth/toaws/command directorywhy
The
authnamespace must remain provider-agnostic per CLI design principles. AWS-specific commands like ECR login belong under theatmos awsnamespace hierarchy, following the established pattern withatmos aws eks update-kubeconfig. This ensures theauthnamespace is not polluted by provider- or service-specific commands and maintains a clean separation between generic auth operations and cloud-specific integrations.references
Closes #ATMOS-37
Acceptance criteria from ATMOS-37:
atmos authSummary by CodeRabbit
New Features
Tests
Documentation
Chores