fix: Terraform shell command not working from Atmos interactive UI#2055
Merged
fix: Terraform shell command not working from Atmos interactive UI#2055
Conversation
…Shell The interactive UI dispatched `terraform shell` through `ExecuteTerraform()`, which passed "shell" as a native terraform subcommand causing it to fail. Route it directly to `ExecuteTerraformShell()` with a dedicated helper. Also adds auth manager integration to terraform shell, extracts testable helpers (shellInfoFromOptions, resolveWorkdirPath, shellOptionsForUI), and adds --identity flag support. Closes #2017 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
aknysh
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2055 +/- ##
==========================================
+ Coverage 75.71% 75.74% +0.03%
==========================================
Files 793 793
Lines 73990 74060 +70
==========================================
+ Hits 56021 56098 +77
+ Misses 14475 14470 -5
+ Partials 3494 3492 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Add tests for edge cases in utils_auth.go: - TestGetMergedAuthConfig_EmptyComponent - TestGetMergedAuthConfig_BothEmpty - TestBuildGlobalAuthSection_LogsLevelOnly - TestBuildGlobalAuthSection_LogsFileOnly - TestMergeGlobalAuthConfig_GlobalAndComponentMerged - Add tests for terraform_shell.go helper functions: - TestShellConfig_Fields - TestShellInfoFromOptions_AllFieldsMapped - TestResolveWorkdirPath_ValidWorkdir - TestResolveWorkdirPath_EmptyMap - TestShellOptionsForUI_DefaultProcessingOptions - Update terraform shell documentation with --identity flag All pure helper functions now have 100% coverage: - shellInfoFromOptions, resolveWorkdirPath, printShellDryRunInfo - shellOptionsForUI, storeAutoDetectedIdentity, buildGlobalAuthSection - getComponentAuthSection, handleMergeError Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai full review please |
github-actions
bot
added
size/xl
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Contributor
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (cmd/terraform/shell.go)
participant Atmos as ExecuteAtmosCmd (internal/exec/atmos.go)
participant Shell as ExecuteTerraformShell (internal/exec/terraform_shell.go)
participant Auth as AuthManager orchestration (internal/exec/utils_auth.go)
participant Stacks as ProcessStacks (internal/exec)
CLI->>CLI: Read --identity flag
CLI->>Atmos: invoke terraform shell command
Atmos->>Atmos: detect "shell" subcommand (UI path)
Atmos->>Shell: route to ExecuteTerraformShell (UI options)
Shell->>Shell: shellInfoFromOptions(opts) / resolveWorkdirPath(...)
Shell->>Auth: createAndAuthenticateAuthManager(atmosConfig, &info)
Auth->>Auth: merge configs, create & authenticate AuthManager
Auth-->>Shell: return AuthManager (+ possibly auto-detected identity)
Shell->>Shell: attach AuthManager & identity to info
Shell->>Stacks: ProcessStacks(info with AuthManager)
Stacks-->>Shell: processing result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
📝 WalkthroughWalkthroughThis PR adds identity-aware authentication support to the Terraform shell command. It introduces an Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Router
participant AuthMgr as Auth Manager
participant Terraform
participant UI
User->>CLI: atmos terraform shell --identity dev-role
CLI->>CLI: Parse --identity flag
CLI->>Router: Execute with ShellOptions(identity=dev-role)
alt UI Flow
User->>UI: Select terraform shell
UI->>Router: Route shell subcommand
Router->>Router: shellOptionsForUI (UI defaults)
else CLI Flow
Router->>Router: Direct to ExecuteTerraformShell
end
Router->>Router: shellInfoFromOptions (convert to ConfigAndStacksInfo)
Router->>AuthMgr: createAndAuthenticateAuthManager
AuthMgr->>AuthMgr: Merge global + component auth config
AuthMgr->>AuthMgr: Authenticate with identity
AuthMgr->>Router: Store identity in ConfigAndStacksInfo
Router->>Terraform: Execute shell with auth context
Terraform-->>User: Shell environment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/exec/terraform_shell_test.go`:
- Around line 198-209: The test TestShellConfig_Fields is tautological because
it only asserts literal assignments on the private struct shellConfig
(componentPath, workingDir, varFile); remove this test or replace it with a
behavior-driven test that exercises the public API that uses shellConfig (call
the constructor/factory or function that returns/operates on shellConfig and
assert observable behavior or outputs), e.g., invoke the exported function that
uses shellConfig and verify its side effects or returned values instead of
asserting on the internal fields directly.
- Around line 211-235: The test TestShellInfoFromOptions_AllFieldsMapped omits
assertions for ProcessingOptions mapping; update the test to verify that
shellInfoFromOptions correctly copies ProcessingOptions from the ShellOptions
input (e.g., assert that info.ProcessTemplates ==
opts.ProcessingOptions.ProcessTemplates, info.ProcessFunctions ==
opts.ProcessingOptions.ProcessFunctions, and info.Skip equals
opts.ProcessingOptions.Skip) so the mapping from ShellOptions ->
ConfigAndStacksInfo is fully covered.
🧹 Nitpick comments (1)
internal/exec/utils_auth.go (1)
98-113: Consider documenting the chain order assumption.The function takes the last element of the chain (
chain[len(chain)-1]). It would help future maintainers to document that the chain is ordered from base identity to final assumed identity.📝 Suggested doc enhancement
// storeAutoDetectedIdentity stores the authenticated identity from AuthManager back into // info.Identity when it was auto-detected (empty). This allows hooks and nested operations // to reuse the identity without re-prompting for credentials. +// The chain is ordered from base to final identity, so we take the last element. func storeAutoDetectedIdentity(authManager auth.AuthManager, info *schema.ConfigAndStacksInfo) {
- Refactor utils_auth.go for testability with dependency injection: - Add componentConfigFetcher and authManagerCreator function types - Create createAndAuthenticateAuthManagerWithDeps with injectable deps - Create getMergedAuthConfigWithFetcher with injectable fetcher - Add comprehensive unit tests for auth orchestration: - TestGetMergedAuthConfigWithFetcher_* (5 tests for various paths) - TestCreateAndAuthenticateAuthManagerWithDeps_* (6 tests) - All pure helper functions now have 100% coverage - Address CodeRabbit review feedback: - Document chain order assumption in storeAutoDetectedIdentity - Remove tautological TestShellConfig_Fields test - Clean up TestShellInfoFromOptions_AllFieldsMapped Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
osterman
reviewed
Feb 5, 2026
osterman
reviewed
Feb 5, 2026
osterman
approved these changes
Feb 5, 2026
|
These changes were released in v1.206.0-rc.1. |
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
terraform shellcommand when invoked from the Atmos interactive UI (TUI)terraform shelldirectly toExecuteTerraformShell()from the UI dispatcher, bypassingExecuteTerraform()AuthManagerpropagation toterraform shellfor YAML functions like!terraform.state--identityflag support toterraform shellcommandshellInfoFromOptions,resolveWorkdirPath,shellOptionsForUI) from inline logiccreateAndAuthenticateAuthManager,getMergedAuthConfig,storeAutoDetectedIdentity) toutils_auth.gowhy
ExecuteTerraform(), which had no handler for theshellsubcommand. Sinceterraform shellis an Atmos-only command (not a native Terraform subcommand), it fell through and attempted to executeterraform shellas a native command, which doesn't exist, resulting in:Terraform has no command named "shell"AuthManagerwas not being passed toProcessStacksin the shell command, causing!terraform.stateand!terraform.outputYAML functions to fail with authentication errors--identityflag was missing fromterraform shell, preventing users from specifying which AWS identity to use for authenticationreferences
Summary by CodeRabbit
New Features
--identityflag (alias-i) to the terraform shell command to specify AWS identity.Improvements
Documentation
--identityflag and examples.Tests