Repository navigation
Fix: atmos auth login "hangs" when run in make targets - #1671
Conversation
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
📝 WalkthroughWalkthroughReplace CI-based prompting guards with terminal-interactivity checks for auth flows, add stdin TTY detection, extend AWS SSO device flow logging and browser prompt behavior, enhance CI detection logging and provider rules, and update developer tooling and mocks to support stdin TTY detection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Term as TTY Detector
participant AuthMgr as Auth Manager
participant AWS as AWS SSO Flow
participant Browser
CLI->>Term: IsTTYForStdin?
alt interactive (stdin is TTY)
CLI->>AuthMgr: GetDefaultIdentity() -> prompt user
AuthMgr->>CLI: show identity prompt
CLI->>AuthMgr: user selects identity
AuthMgr->>AWS: Authenticate() (SSO)
AWS->>AWS: register client -> start device auth (debug logs)
AWS->>CLI: display VerificationUri and code
AWS->>Browser: open(VerificationUri) (attempt)
Browser-->>AWS: open success/fail (logged)
else non-interactive (not TTY)
CLI->>AuthMgr: GetDefaultIdentity() -> return no default or error
CLI->>AWS: Authenticate() -> error (requires interactive)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 (2)
pkg/auth/manager.go (1)
31-35: Consider adding perf tracking for consistency.The coding guidelines suggest adding
defer perf.Track(nil, "auth.isInteractive")()to critical private functions. While this is a trivial helper, it's used in authentication flows. That said, the overhead might outweigh the benefit for such a simple check.pkg/auth/providers/aws/sso.go (1)
149-150: Comment may need clarification.The comment says the prompt is shown "unless we're in a non-interactive environment," but the code always displays it (no conditional check). This might be intentional if
isInteractive()checks inmanager.goalready gate authentication, but clarifying the comment would help future maintainers.
📜 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 (3)
pkg/auth/manager.go(4 hunks)pkg/auth/providers/aws/sso.go(4 hunks)pkg/telemetry/ci.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/providers/aws/sso.gopkg/telemetry/ci.gopkg/auth/manager.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must be complete sentences ending with periods.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines, sorted alphabetically within each group; preserve existing aliases.
Wrap all returned errors using static errors from errors/errors.go; prefer errors.Join for multiple errors; use fmt.Errorf with %w for adding context; check with errors.Is; never compare err.Error().
Use viper.BindEnv for environment variable binding; every env var must have an ATMOS_ alternative.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; do not use logging for UI.
Most text UI must go to stderr; only data/results go to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
Prefer interfaces for multi-provider capabilities and generate mocks for them; use provider registry pattern.
Ensure cross-platform compatibility: use stdlib (filepath, os.PathSeparator), avoid platform-specific binaries when SDKs exist, and guard OS-specific logic with runtime.GOOS.
Files:
pkg/auth/providers/aws/sso.gopkg/telemetry/ci.gopkg/auth/manager.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track(<atmosConfig or nil>, "packagename.FunctionName")()at the start of all public and critical private functions, followed by a blank line.
Files:
pkg/auth/providers/aws/sso.gopkg/telemetry/ci.gopkg/auth/manager.go
🧬 Code graph analysis (2)
pkg/auth/providers/aws/sso.go (6)
pkg/auth/cloud/aws/resolver.go (1)
GetResolverConfigOption(25-49)pkg/logger/log.go (2)
Debug(24-26)Errorf(59-61)errors/errors.go (1)
ErrAuthenticationFailed(335-335)pkg/telemetry/ci.go (1)
IsCI(79-89)pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)pkg/utils/url_utils.go (1)
OpenUrl(13-39)
pkg/telemetry/ci.go (1)
pkg/logger/log.go (1)
Debug(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (10)
pkg/auth/manager.go (2)
204-206: Good use of terminal detection over CI detection.Checking
isInteractive()is more accurate thantelemetry.IsCI()for determining whether to prompt. This prevents hangs in truly non-interactive environments while allowing prompts even when CI env vars are set (e.g., running make locally).
216-218: Consistent interactive check.Same improvement as line 204—using TTY detection ensures prompts only appear when appropriate.
pkg/telemetry/ci.go (5)
44-48: Excellent fix for Jenkins false positives.Requiring both
JENKINS_URLandBUILD_IDprevents false detection when build-harness sets onlyJENKINS_URL. This is exactly the right approach.
80-88: Debug logging adds helpful visibility.The debug output will make it much easier to diagnose CI detection issues. Good addition.
169-190: Well-structured detection priority.Checking stricter rules (ALL env vars) before looser rules (single env var) is the right approach. The alphabetical sorting ensures deterministic results, and the debug logging will help troubleshoot detection issues.
194-200: Enhanced logging aids debugging.Logging both the detected env var name and its value provides useful context for understanding why a particular CI provider was detected.
216-222: Comprehensive value-based detection logging.The debug logs showing expected vs. actual values will be helpful when debugging CI detection behavior.
pkg/auth/providers/aws/sso.go (3)
83-85: Critical fix for authentication hangs.Using
aws.AnonymousCredentials{}prevents the SDK from attempting to load credentials from the default chain, which can hang on metadata endpoints. This is exactly right for SSO device flow, which doesn't require pre-existing credentials.
93-125: Comprehensive authentication flow logging.The debug logs at each major step (config load, client registration, device authorization) will make it much easier to diagnose where authentication issues occur.
157-170: Good improvements to prompt flow.Removing CI-gating for the browser prompt makes sense—users running
makelocally with CI env vars set will appreciate seeing the prompt. The corrected message text ("verify code") is more accurate, and the debug logging provides good visibility into the authentication flow.
- Use TTY detection instead of telemetry CI for runtime decisions - Add IsTTYSupportForStdin() for standardized interactive checks - Fix Jenkins detection to require JENKINS_URL AND BUILD_ID - SSO auth fails fast in headless with helpful error - Fix prompt message: 'verify code' not 'enter code' 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…orruption ## what - Change SSO auth interactive detection from stdin to stderr - Update pre-commit hook to check for custom-gcl binary instead of building it - Add script to check for custom-gcl before running golangci-lint - Add constant for "provider" log key to satisfy revive linter ## why - SSO device flow outputs authentication URL to stderr, not stdin - Make doesn't redirect stdin, so stdin TTY check always passes even in make targets - Need to check stderr TTY because that's where the user sees the auth instructions - Building custom-gcl in pre-commit can cause git corruption in worktrees - Pre-commit should fail fast with helpful message instead of building - Linter requires constants for repeated string literals 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## what - Consolidate from 2 scripts to 1 run script - Simplify build script to build in project root (no temp directory) - Update pre-commit to use run script that checks for binary ## why - Don't need separate check script when run script can do the same check - Building in temp directory was unnecessary complexity - Simpler workflow: users run `make custom-gcl` once, then commits work - Clearer separation: build script builds, run script runs (with check) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## what - Move build logic from script directly into Makefile target - Delete scripts/build-custom-golangci-lint.sh ## why - Single line command doesn't need a separate script file - Simpler to maintain build logic in one place - Fewer files to manage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## what - Add prominent comment in pre-commit config explaining why NOT to auto-build ## why - Prevent future developers from "helpfully" adding automatic build - Document the git corruption issue that can occur - Make it clear this is intentional, not an oversight 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
## what - Merge latest main branch changes - Resolve conflict in pkg/auth/providers/aws/sso.go ## why - Keep branch up to date with main - Use improved LoadIsolatedAWSConfig from main - Preserve debug logging from our branch 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
changed the title
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/run-custom-golangci-lint.sh (1)
13-13: Consider making lint arguments configurable.The hardcoded
--new-from-rev=origin/main --config=.golangci.ymlflags work for the standard case, but allowing arguments to be passed through ("$@") would provide more flexibility for manual runs.If you want more flexibility, apply this diff:
-./custom-gcl run --new-from-rev=origin/main --config=.golangci.yml +./custom-gcl run --new-from-rev=origin/main --config=.golangci.yml "$@"This allows users to pass additional flags when running the script manually.
📜 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 (10)
.golangci.yml(1 hunks).pre-commit-config.yaml(1 hunks)Makefile(1 hunks)internal/tui/templates/term/mock_term_writer.go(2 hunks)internal/tui/templates/term/term_writer.go(3 hunks)pkg/auth/manager.go(4 hunks)pkg/auth/providers/aws/sso.go(6 hunks)pkg/telemetry/ci.go(5 hunks)scripts/build-custom-golangci-lint.sh(0 hunks)scripts/run-custom-golangci-lint.sh(1 hunks)
💤 Files with no reviewable changes (1)
- scripts/build-custom-golangci-lint.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/telemetry/ci.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must be complete sentences ending with periods.
Organize imports in three groups (stdlib, third-party, Atmos) separated by blank lines, sorted alphabetically within each group; preserve existing aliases.
Wrap all returned errors using static errors from errors/errors.go; prefer errors.Join for multiple errors; use fmt.Errorf with %w for adding context; check with errors.Is; never compare err.Error().
Use viper.BindEnv for environment variable binding; every env var must have an ATMOS_ alternative.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; do not use logging for UI.
Most text UI must go to stderr; only data/results go to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
Prefer interfaces for multi-provider capabilities and generate mocks for them; use provider registry pattern.
Ensure cross-platform compatibility: use stdlib (filepath, os.PathSeparator), avoid platform-specific binaries when SDKs exist, and guard OS-specific logic with runtime.GOOS.
Files:
internal/tui/templates/term/mock_term_writer.gopkg/auth/manager.gointernal/tui/templates/term/term_writer.gopkg/auth/providers/aws/sso.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track(<atmosConfig or nil>, "packagename.FunctionName")()at the start of all public and critical private functions, followed by a blank line.
Files:
internal/tui/templates/term/mock_term_writer.gopkg/auth/manager.gointernal/tui/templates/term/term_writer.gopkg/auth/providers/aws/sso.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/auth/manager.gopkg/auth/providers/aws/sso.go
.golangci.yml
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Configure golangci-lint with gofmt, goimports, govet, staticcheck, errcheck, ineffassign, misspell, unused, revive, gocritic enabled
Files:
.golangci.yml
🧠 Learnings (4)
📚 Learning: 2025-10-11T19:11:58.965Z
Learnt from: osterman
PR: cloudposse/atmos#1599
File: internal/exec/terraform.go:0-0
Timestamp: 2025-10-11T19:11:58.965Z
Learning: For terraform apply interactivity checks in Atmos (internal/exec/terraform.go), use stdin TTY detection (e.g., `IsTTYSupportForStdin()` or checking `os.Stdin`) to determine if user prompts are possible. This is distinct from stdout/stderr TTY checks used for output display (like TUI rendering). User input requires stdin to be a TTY; output display requires stdout/stderr to be a TTY.
Applied to files:
pkg/auth/manager.gointernal/tui/templates/term/term_writer.go
📚 Learning: 2025-09-13T18:06:07.674Z
Learnt from: samtholiya
PR: cloudposse/atmos#1466
File: toolchain/list.go:39-42
Timestamp: 2025-09-13T18:06:07.674Z
Learning: In the cloudposse/atmos repository, for UI messages in the toolchain package, use utils.PrintfMessageToTUI instead of log.Error or fmt.Fprintln(os.Stderr, ...). Import pkg/utils with alias "u" to follow the established pattern.
Applied to files:
pkg/auth/manager.gopkg/auth/providers/aws/sso.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
PR: cloudposse/atmos#1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/auth/manager.go
📚 Learning: 2024-11-10T18:37:10.032Z
Learnt from: osterman
PR: cloudposse/atmos#768
File: internal/exec/vendor_component_utils.go:354-360
Timestamp: 2024-11-10T18:37:10.032Z
Learning: In the vendoring process, a TTY can exist without being interactive. If the process does not prompt the user, we should not require interactive mode to display the TUI. The `CheckTTYSupport` function should check TTY support on stdout rather than stdin.
Applied to files:
internal/tui/templates/term/term_writer.go
🧬 Code graph analysis (2)
pkg/auth/manager.go (1)
internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStdin(133-135)
pkg/auth/providers/aws/sso.go (6)
internal/tui/templates/term/term_writer.go (1)
IsTTYSupportForStderr(127-129)errors/errors.go (1)
ErrAuthenticationFailed(335-335)pkg/auth/cloud/aws/resolver.go (1)
GetResolverConfigOption(25-49)pkg/telemetry/ci.go (1)
IsCI(80-90)pkg/utils/log_utils.go (1)
PrintfMessageToTUI(33-35)pkg/utils/url_utils.go (1)
OpenUrl(13-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (16)
Makefile (1)
32-34: Inline build approach looks good.The transition from an external script to inline build steps is clear and maintainable. The progress messages and GOFLAGS setting are appropriate.
.golangci.yml (1)
65-66: Updated guidance covers all TTY detection functions.The expanded message now includes stdin alongside stdout/stderr, properly reflecting the broader TTY detection surface introduced in this PR.
.pre-commit-config.yaml (1)
29-36: Pre-commit workflow improvement looks solid.The separation of build and execution phases prevents git corruption issues, and the clear comments help maintainers understand the constraint.
pkg/auth/manager.go (3)
30-34: Interactive detection is properly implemented.The
isInteractive()helper correctly uses stdin TTY detection for determining whether user prompts are possible. This aligns with the distinction between input (stdin) and output (stdout/stderr) TTY checks.Based on learnings
203-207: Correct behavior for non-interactive environments.Returning an error when no default identity exists and the environment is non-interactive prevents hanging on prompts that cannot be answered.
215-219: Multiple defaults handled appropriately.In non-interactive mode, the error clearly indicates the ambiguity rather than attempting an impossible prompt.
internal/tui/templates/term/term_writer.go (3)
20-21: Interface extension follows established patterns.Adding
IsTTYForStdin()to theTTYDetectorinterface maintains consistency with the existing stdout/stderr methods.
39-43: Stdin TTY detection implemented correctly.The implementation properly checks
os.Stdin.Fd()withterm.IsTerminal(), following the same pattern as stdout and stderr detection.
131-135: Public convenience function is consistent.
IsTTYSupportForStdin()follows the same pattern as the existing stdout/stderr variants, delegating to the default detector.internal/tui/templates/term/mock_term_writer.go (1)
50-62: Mock support for stdin TTY detection.The generated mock methods for
IsTTYForStdin()follow the standard gomock pattern and enable testing of the new interactivity checks.pkg/auth/providers/aws/sso.go (6)
28-33: Interactive check uses stderr appropriately for device flow.For AWS SSO device flow, checking stderr TTY makes sense because the authentication instructions must be visible to the user. The device flow doesn't require stdin input—the user completes authentication in a browser.
88-91: Proper guard against headless environments.Failing early with a clear error message when no TTY is detected prevents the authentication flow from hanging or behaving unexpectedly in CI/automated environments.
96-98: Critical fix: AnonymousCredentials prevents hangs.Explicitly configuring
aws.AnonymousCredentials{}prevents the AWS SDK from attempting to load credentials from default providers (EC2 metadata, ECS task role, etc.), which could hang or cause timeouts in non-interactive environments. This is essential for the SSO device flow.
106-139: Debug logging improves observability.The added debug logs at key stages (loading config, registering client, starting device auth) make troubleshooting authentication flows much easier.
171-183: Improved prompt behavior for mixed environments.Always showing the authentication prompt and attempting browser launch is better than the previous CI-only logic, especially for developers running
makelocally with CI environment variables set.
176-176: Messaging correction: "verify code" is more accurate.The user needs to verify the code shown, not enter it, making this wording clearer.
## what - Remove noisy "CI environment detected" logging from IsCI() - Change debug logging format to show env vars: provider=JENKINS env=JENKINS_URL,BUILD_ID - Fix Jenkins test to set both JENKINS_URL and BUILD_ID (required for detection) - Update telemetry test to set both env vars ## why - IsCI() was logging even when provider was empty (noisy in test output) - New format shows exactly which env vars were detected - Jenkins detection requires both JENKINS_URL and BUILD_ID to avoid false positives - Tests need to match the actual detection logic 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1671 +/- ##
==========================================
- Coverage 66.39% 66.27% -0.12%
==========================================
Files 350 350
Lines 39606 39670 +64
==========================================
- Hits 26296 26292 -4
- Misses 11299 11373 +74
+ Partials 2011 2005 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Andriy Knysh (aknysh)
added
minor
|
Warning Changelog Entry RequiredThis PR is labeled Action needed: Add a new blog post in Example filename: Alternatively: If this change doesn't require a changelog entry, remove the |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/telemetry/ci.go (1)
88-127: PreserveCIEnvVars should handle the new all-exist providers map.The function preserves environment variables from
ciProvidersEnvVarsExistsandciProvidersEnvVarsEqualsbut doesn't iterate over the newly addedciProvidersEnvVarsAllExistmap. This creates inconsistency and could lead to test isolation issues ifJENKINS_URLorBUILD_IDare set externally.Apply this diff to preserve all-exist provider variables:
func PreserveCIEnvVars() map[string]string { // Initialize map to store original environment variable values envVars := make(map[string]string) // Preserve and unset CI provider variables that are detected by existence. for _, envVar := range ciProvidersEnvVarsExists { if isEnvVarExists(envVar) { envVars[envVar] = os.Getenv(envVar) //nolint:forbidigo // Legitimate use for CI env preservation os.Unsetenv(envVar) } } + // Preserve and unset CI provider variables that require all to exist. + for _, vars := range ciProvidersEnvVarsAllExist { + for _, envVar := range vars { + if isEnvVarExists(envVar) { + envVars[envVar] = os.Getenv(envVar) //nolint:forbidigo // Legitimate use for CI env preservation + os.Unsetenv(envVar) + } + } + } + // Preserve and unset CI provider variables that are detected by specific values. for _, values := range ciProvidersEnvVarsEquals { for valueKey := range values { if isEnvVarExists(valueKey) { envVars[valueKey] = os.Getenv(valueKey) os.Unsetenv(valueKey) } } } // Preserve and unset the general CI environment variable. if isEnvVarExists(ciEnvVar) { envVars[ciEnvVar] = os.Getenv(ciEnvVar) os.Unsetenv(ciEnvVar) } return envVars }Also update the comment to reflect four categories instead of three.
🧹 Nitpick comments (1)
pkg/telemetry/ci.go (1)
209-224: Value-based detection enhanced with detailed logging.The logging collects all detected environment variables and joins them for readability. The defensive check for
len(detectedVars) > 0on line 219 is technically redundant (sinceresult != ""guarantees at least one match), but it's harmless and adds safety.If you want to simplify, the length check could be removed since the result already confirms a match:
if result := applyAlphabeticalOrder(ciProvidersEnvVarsEquals, checkEnvVarsEquals); result != "" { if envVars, exists := ciProvidersEnvVarsEquals[result]; exists { var detectedVars []string for envName := range envVars { if _, found := os.LookupEnv(envName); found { detectedVars = append(detectedVars, envName) } } - if len(detectedVars) > 0 { - log.Debug("CI provider detected", logKeyProvider, result, "env", strings.Join(detectedVars, ",")) - } + log.Debug("CI provider detected", logKeyProvider, result, "env", strings.Join(detectedVars, ",")) } return result }
📜 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 (3)
pkg/telemetry/ci.go(5 hunks)pkg/telemetry/ci_test.go(1 hunks)pkg/telemetry/utils_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.gopkg/telemetry/ci.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Unit tests should be table-driven where appropriate and focus on pure functions; target >80% coverage with emphasis on pkg/ and internal/exec/.
Test behavior, not implementation; avoid tautological or stub-only tests; use dependency injection to make code testable; remove always-skipped tests; table-driven tests must use realistic scenarios.
Place//go:generate mockgendirectives for mocks at the top of test files; for internal interfaces use-source=$GOFILE -destination=mock_$GOFILE -package=$GOPACKAGE.
Tests must call production code paths (do not duplicate production logic within tests).
Always use t.Skipf with a reason (never t.Skip or Skipf without context).
Test files should mirror implementation structure and be co-located with source files (foo.go ↔ foo_test.go).
Use precondition-based test skipping helpers from tests/test_preconditions.go (e.g., RequireAWSProfile, RequireGitHubAccess).
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods; enforced by golangci-lint godot across all Go comments.
Organize imports into three groups (stdlib, third-party, Atmos) separated by blank lines and sorted alphabetically within each group; keep existing aliases.
All errors must be wrapped using static errors defined in errors/errors.go; prefer errors.Join for multiple, fmt.Errorf with %w for context, and errors.Is for checks; never rely on string comparisons.
Prefer cross-platform implementations: use SDKs over external binaries; use filepath/os facilities; gate OS-specific logic with runtime.GOOS or build tags.
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.gopkg/telemetry/ci.go
{cmd,internal,pkg}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
{cmd,internal,pkg}/**/*.go: Adddefer perf.Track()to all public functions and critical private ones, include a blank line after it, and use package-qualified names (e.g., "exec.ProcessComponent"). Use atmosConfig if available, else nil.
Always bind environment variables with viper.BindEnv; every var must have an ATMOS_ alternative and prefer ATMOS_ over external names.
Distinguish structured logging from UI output: UI prompts/errors/status to stderr; data/results to stdout; logging for system/debug only; no UI via logging.
Most text UI must go to stderr (via utils.PrintfMessageToTUI or fmt.Fprintf(os.Stderr,...)); only data/results to stdout.
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.gopkg/telemetry/ci.go
{pkg,internal,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always use mockgen for interface mocks; never write manual mocks with many stub methods.
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.gopkg/telemetry/ci.go
pkg/{,**/}**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should primarily cover pkg/ code; ensure meaningful coverage with real scenarios (not coverage theater).
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.go
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Target minimum 80% coverage on new/changed lines; exclude mock files from coverage: **/mock_.go, mock_.go, **/mock/*.go.
Files:
pkg/telemetry/utils_test.gopkg/telemetry/ci_test.gopkg/telemetry/ci.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
pkg/telemetry/ci.go
🧬 Code graph analysis (1)
pkg/telemetry/ci.go (1)
pkg/logger/log.go (1)
Debug(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (8)
pkg/telemetry/ci_test.go (1)
147-153: Test data correctly updated for new Jenkins detection logic.The addition of
BUILD_IDaligns perfectly with the enhanced Jenkins detection that now requires bothJENKINS_URLandBUILD_IDto prevent false positives.pkg/telemetry/utils_test.go (1)
310-313: Jenkins simulation correctly updated.The
BUILD_IDenvironment variable completes the Jenkins CI simulation, matching the new detection requirements.pkg/telemetry/ci.go (6)
3-9: Imports appropriately added for logging enhancements.The
stringspackage supports readable log output and thelogalias enables debug logging throughout CI detection.
11-14: Constants properly defined.Using
logKeyProviderensures consistent log keys across all debug statements.
46-50: All-exist detection map properly designed.The new map structure cleanly handles providers requiring multiple environment variables, and the comment provides helpful context about avoiding build-harness false positives.
79-86: IsCI() logic improved for clarity.Extracting the two conditions into named variables makes the function's logic more explicit and maintainable.
166-187: All-exist provider detection correctly implemented.The logic properly checks that all required environment variables exist before returning a provider match. Alphabetical sorting ensures deterministic behavior, and the debug logging will be helpful for troubleshooting.
189-197: Existence-based detection enhanced with logging.The debug logging addition provides visibility into which environment variable triggered the detection, improving debuggability without changing the core logic.
|
These changes were released in v1.195.0-rc.1. |
The previous fix in #1654 only isolated environment variables but the AWS SDK still loaded from ~/.aws/config and ~/.aws/credentials by default. This caused authentication failures when users had AWS_PROFILE set in their shell. The error 'failed to get shared config profile, cplive-core-gbl-identity' occurred because the SDK tried to load that profile from shared config files even though AWS_PROFILE was cleared from the environment. Solution: Use config.WithSharedConfigProfile("") to explicitly disable shared config loading in LoadIsolatedAWSConfig(). This ensures the SDK only uses credentials provided programmatically by Atmos. References: DEV-3706, #1671
* Changes auto-committed by Conductor * [autofix.ci] apply automated fixes * feat: Make AWS credentials directory configurable via provider spec Add support for configuring the AWS credentials base directory path through provider spec configuration with proper precedence handling. - Add files.base_path configuration option to provider spec - Update AWSFileManager to accept optional base_path parameter - Implement precedence: spec config > env var > default ~/.aws/atmos - Add GetFilesBasePath helper to extract path from provider spec - Update all providers (SSO, SAML) to use configured base_path - Add GetFilesDisplayPath method to AuthManager interface - Support tilde expansion for user home directory paths - Use viper.GetString for environment variable access - Update golden snapshot for auth invalid-command test This allows users to customize where AWS credential files are stored on a per-provider basis, enabling XDG-compliant paths or custom locations while maintaining backward compatibility with the default ~/.aws/atmos directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Display actual configured paths in auth logout dry-run output Updated the `atmos auth logout` command to show the actual configured AWS files path instead of hardcoded `~/.aws/atmos/` in dry-run mode. Changes: - performIdentityLogout: Use authManager.GetFilesDisplayPath() for provider path - performProviderLogout: Use authManager.GetFilesDisplayPath() for provider path - performLogoutAll: Enhanced to show all provider paths using GetFilesDisplayPath() This ensures users see the correct path whether using the default ~/.aws/atmos or a custom path configured via spec.files.base_path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add validation for spec.files.base_path in AWS providers Added validation to ensure spec.files.base_path is properly formatted when configured in AWS auth providers (SSO and SAML). Changes: - Created ValidateFilesBasePath() function in pkg/auth/cloud/aws/spec.go - Updated SSO and SAML provider Validate() methods to call validation - Added comprehensive tests covering valid and invalid path scenarios - Validation checks for: empty/whitespace paths, invalid characters This ensures configuration errors are caught early during auth validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Add spec.files.base_path configuration documentation Updated documentation to explain the configurable AWS files base path feature for auth logout command. Changes: - PRD: Added Configuration section with spec.files.base_path examples - PRD: Updated FR-004 to mention configurable base path - CLI docs: Added "Advanced Configuration" section with full examples - CLI docs: Documented configuration precedence and validation - CLI docs: Added example with environment variable override The documentation explains: - How to configure custom file paths in atmos.yaml - Configuration precedence (config > env var > default) - Path validation rules - Use cases for custom paths (containers, multi-user, etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: Add perf tracking, remove unused error, remove env var support Made several code quality improvements to auth logout implementation: 1. Performance Tracking: - Added perf.Track to executeAuthLogoutCommand in cmd/auth_logout.go - Added perf.Track to GetFilesBasePath in pkg/auth/cloud/aws/spec.go - Ensures performance monitoring for auth logout operations 2. Dead Code Removal: - Removed ErrLogoutNotSupported from errors/errors.go (never used) - All providers implement Logout, no concept of unsupported logout exists 3. Environment Variable Removal: - Removed ATMOS_AWS_FILES_BASE_PATH env var support per requirements - Only spec.files.base_path configuration is supported now - Removed viper import from pkg/auth/cloud/aws/files.go - Updated documentation to remove env var references 4. Documentation Updates: - Updated PRD to remove env var from precedence - Updated CLI docs to remove env var example and precedence - Simplified configuration to: spec config > default Changes maintain backward compatibility - default behavior unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add logout not supported vs not implemented distinction - Add ErrLogoutNotSupported (exit 0) and ErrLogoutNotImplemented (exit non-zero) - GitHub OIDC returns ErrLogoutNotSupported (no local storage to clean) - Manager treats ErrLogoutNotSupported as success - Fix duplicate identity.Logout calls with identityLogoutCalled flag - Add resolveProviderForIdentity for transitive Via chain resolution - Update LogoutProvider to find identities transitively - Add comprehensive tests (79.5% coverage in pkg/auth) * refactor: Fix N+1 provider cleanup and improve error handling in logout - Add ErrPartialLogout handling to cmd/auth_logout.go (treat as success/exit 0) - Fix N+1 provider.Logout calls in LogoutProvider using context flag - Treat keyring.ErrNotFound as success in credentialStore.Delete - Fix test map types to use types.Provider and types.Identity - Update test expectations to expect Times(1) for provider operations - Add comprehensive test for LogoutProvider with failures - Test coverage increased from 79.6% to 80.8% * fix: Add identity-scoped cleanup to preserve other identities' credentials - Add CleanupIdentity() method to AWSFileManager - Remove only specific identity's sections from shared INI files - Prevent deletion of other identities' credentials when logging out one identity - Add removeIniSection() helper to safely remove INI sections - Update aws-user identity to use CleanupIdentity instead of Cleanup This fixes the critical issue where logging out one aws-user identity would delete credentials for all identities using the same provider. * test: Update TestDelete_Flow to expect success for non-existent credentials - Delete now treats keyring.ErrNotFound as success (idempotent) - Updated test to assert.NoError instead of assert.Error - Added test for deleting already-deleted credential (idempotent behavior) * feat: Add basePath parameter to AWS setup functions - Add basePath parameter to SetupFiles() and SetEnvironmentVariables() - Update all callers to pass empty string for now (maintains default behavior) - Supports provider's configured files.base_path in future This addresses the code review comment about setup.go ignoring provider's configured files.base_path. The functions now accept basePath but callers currently pass empty string to maintain default ~/.aws/atmos behavior. Future work: Resolve actual files.base_path from provider config and pass it. * test: Add comprehensive tests for auth logout functionality - Add tests for cmd/auth_logout.go (performIdentityLogout, performProviderLogout, performLogoutAll) - Add tests for pkg/auth/cloud/aws/files.go (CleanupIdentity, removeIniSection) - Fix CleanupIdentity to use 'profile <name>' format for config file sections - Add test for 'default' identity special case handling Coverage improvements: - cmd/auth_logout.go: significantly improved from 1.77% - pkg/auth/cloud/aws/files.go: improved from 12.82% * test: Add Logout tests for AWS and GitHub providers - Add tests for SAML and SSO provider Logout methods - Add test for GitHub OIDC provider Logout (ErrLogoutNotSupported) - Verify Logout works with custom base_path configuration Coverage improvements: - pkg/auth/providers/aws/saml.go: improved Logout coverage - pkg/auth/providers/aws/sso.go: improved Logout coverage - pkg/auth/providers/github/oidc.go: improved Logout coverage * [autofix.ci] apply automated fixes * test: Add comprehensive logout tests for manager and identities - Add manager logout tests for edge cases: - Identity in chain (lines 789-803 coverage) - Identity logout returning ErrLogoutNotSupported - Identity in chain with logout failure - LogoutAll with mixed success/failure - Add identity logout tests: - AssumeRoleIdentity Logout (returns nil) - PermissionSetIdentity Logout (returns nil) - UserIdentity Logout (calls CleanupIdentity) Coverage improvements: - pkg/auth/manager.go: improved from 73.57% to near 80%+ - pkg/auth/identities/aws/assume_role.go: added Logout coverage - pkg/auth/identities/aws/permission_set.go: added Logout coverage - pkg/auth/identities/aws/user.go: added Logout coverage * [autofix.ci] apply automated fixes * Changes auto-committed by Conductor * Changes auto-committed by Conductor * Add performance instrumentation and error classification to AWS providers - Add perf.Track() instrumentation to SSO provider methods: - Validate() for validation performance tracking - Logout() for cleanup operation tracking - GetFilesDisplayPath() for path resolution tracking - Add perf.Track() instrumentation to SAML provider methods: - Logout() for cleanup operation tracking - GetFilesDisplayPath() for path resolution tracking - Update error classification in Logout methods: - Use errors.Join(ErrProviderLogout, ErrLogoutFailed, err) for proper error hierarchy - Enables better error handling and troubleshooting 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix Windows path separator compatibility in GetFilesDisplayPath tests - Normalize path separators using filepath.ToSlash() before comparison - Add path/filepath import to test files - Fixes test failures on Windows where paths use backslashes Windows was returning "~\.aws\atmos" but tests expected "~/.aws/atmos". Using filepath.ToSlash() normalizes to forward slashes for comparison. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add performance instrumentation to SAML provider Validate method - Add perf.Track() to Validate() for performance measurement - Consistent with instrumentation pattern used in other provider methods 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Changes auto-committed by Conductor * Ignore merge conflict artifacts in gitignore - Add *.orig and *.rej to .gitignore - Remove tracked saml.go.orig file from repository These files are generated by patch/merge tools and should not be committed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add performance instrumentation to AWS identity Logout methods - Add perf.Track() to assumeRoleIdentity.Logout() - Add perf.Track() to permissionSetIdentity.Logout() - Add perf.Track() to userIdentity.Logout() - Fix comment punctuation (all comments now end with periods) All three identity types now have consistent performance tracking for their Logout methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Add test coverage for auth components Increases test coverage across multiple auth packages: **pkg/auth/cloud/aws/files_test.go:** - Add tests for GetBaseDir() - 2 test cases - Add tests for GetDisplayPath() - 3 test cases with tilde expansion - All tests use cross-platform path normalization **pkg/auth/credentials/store_test.go:** - Add tests for SetAny() - 3 test cases - Test complex struct marshaling - Test overwriting existing values **pkg/auth/providers/aws:** - Add SAML Logout error path test (invalid base_path) - Add SSO Logout error path test (invalid base_path) **pkg/auth/identities/aws/user_test.go:** - Add test for Validate() method Coverage improvements: - pkg/auth/cloud/aws: 76.2% → 78.6% (+2.4%) - pkg/auth/providers/aws: 59.8% → 61.3% (+1.5%) - pkg/auth/credentials: 87.5% (maintained) Total: 11 new test cases added 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Improve logout UI with concise, modern messaging Replace verbose two-step logout messages with single-line feedback: **Before:** ``` Logging out from identity: example ✓ Successfully logged out ``` **After:** ``` ✓ Logged out **example** ``` Changes: - Remove "Logging out from..." preamble messages - Show result with green checkmark (✓) on one line - Include entity name and count in success message - More modern, friendly UI matching Terraform clean pattern Examples: - Identity: `✓ Logged out **identity-name**` - Provider: `✓ Logged out provider **aws-sso** (3 identities)` - All: `✓ Logged out all 5 identities` Error messages also updated for consistency: - `✗ Failed to log out **identity-name**` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Use PrintfMarkdownToTUI for all markdown-formatted messages Replace PrintfMessageToTUI with PrintfMarkdownToTUI for messages that use markdown formatting (**bold** syntax). This ensures: - Markdown is properly rendered instead of showing raw ** symbols - Line wrapping is handled automatically by the markdown renderer - No need for manual line breaks with \n Changes: - Error messages with **Error:** prefix - Headers like **Available identities:** - **Dry run mode:** notifications - Success messages with **bold** entity names - Warning messages like **Provider logout partially succeeded** - Browser session warning with **Note:** prefix The markdown renderer will handle formatting and line wrapping, making the output cleaner and more professional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Show browser session warning only once using cache Reduce noise by displaying the browser session warning only on the first logout. Uses Atmos cache mechanism to track if warning has been shown, similar to how telemetry disclosure is handled. Changes: - Add BrowserSessionWarningShown field to CacheConfig - Update displayBrowserWarning() to check cache before showing - Mark warning as shown in cache after first display - Gracefully handle cache errors (warning still shows if cache fails) Behavior: - First logout: Shows warning and marks as shown in cache - Subsequent logouts: Silently skips warning - Cache location: ~/.cache/atmos/cache.yaml (respects XDG) This makes repeated logout operations much less noisy while ensuring users still see the important browser session notice at least once. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [autofix.ci] apply automated fixes * Fix AWS env isolation to prevent shared config file loading The previous fix in #1654 only isolated environment variables but the AWS SDK still loaded from ~/.aws/config and ~/.aws/credentials by default. This caused authentication failures when users had AWS_PROFILE set in their shell. The error 'failed to get shared config profile, cplive-core-gbl-identity' occurred because the SDK tried to load that profile from shared config files even though AWS_PROFILE was cleared from the environment. Solution: Use config.WithSharedConfigProfile("") to explicitly disable shared config loading in LoadIsolatedAWSConfig(). This ensures the SDK only uses credentials provided programmatically by Atmos. References: DEV-3706, #1671 * Fix auth logout tests to expect GetIdentities and GetProviderForIdentity calls The UI improvement in ac9fe05 added code to count identities per provider for better logout messages. This requires calling GetIdentities() and GetProviderForIdentity() in performProviderLogout(). Updated mock expectations in tests to match the actual code flow: - Added GetIdentities() mock for provider logout tests - Added GetProviderForIdentity() mock to determine which identities use the provider - Added GetIdentities() mock for successful logout all (to show count) - Correctly excluded GetIdentities() from partial logout all (returns early) * Fix AWS setup tests using homedir.Reset() to clear cache The tests were failing in CI because the homedir package caches the home directory value. When tests use t.Setenv("HOME"), the cached value isn't updated. Proper fix: Call homedir.Reset() after setting HOME to clear the cache. This forces homedir.Dir() to re-detect the home directory from the updated environment variable. This approach: - Uses the intended homedir cache management API - Tests the actual default path behavior (empty basePath parameter) - Is more realistic than passing explicit paths - Matches the pattern documented in PR #1673 * Changes auto-committed by Conductor * Replace deprecated github.com/golang/mock with go.uber.org/mock - Update all mock files and test files to use go.uber.org/mock/gomock - Update go.mod and go.sum with new dependency - Fixes CI compilation errors from deprecated gomock package - Note: Skipping golangci-lint for pre-existing issues in auth code * Add perf instrumentation and fix import ordering - Add perf tracking to Cleanup, CleanupIdentity, and CleanupAll in pkg/auth/cloud/aws/files.go - Add context parameter to CleanupIdentity signature and update all callers - Fix import ordering in validate_schema_test.go and version_test.go per project guidelines - Fix missing period in version_test.go comment - Add missing context import to files_test.go - All tests passing * Add blog post for atmos auth shell with corrected problem statement - Reposition problem statement to focus on secure multi-identity workflows - Emphasize session scoping, credential isolation, and security benefits - Compare with AWS Vault's approach to credential management - Add clear guidance on when to use 'auth shell' vs 'auth env' - Include real-world workflows and multi-identity scenarios - Fix all documentation links to use correct URL patterns * Fix missing periods in blog post list items - Add terminal punctuation to all list items in 'How It Works' section - Ensures consistent punctuation throughout the document * Remove auth shell blog post and add auth logout blog post - Remove auth shell blog post (doesn't belong in logout PR) - Add auth logout blog post focusing on general cloud tooling problem - Position logout as solving credential cleanup difficulty across all providers - Emphasize Atmos Auth has only been out for less than a week - Frame problem as 'cloud tooling doesn't make logout easy' not 'Atmos lacked logout' * Remove duplicate auth logout blog post - Already have 2025-10-17-auth-logout-feature.md - Shouldn't have created a duplicate * Update auth logout blog post problem statement - Reframe problem: cloud tooling doesn't make logout easy - Focus on credential sprawl across AWS, Azure, GCP - Emphasize manual cleanup burden and security risks - Position Atmos logout as solving general cloud tooling gap - Remove implication that Atmos previously lacked logout * Update auth logout documentation with improved problem statement - Add context about cloud tooling not making logout easy - Mention credential sprawl across AWS, Azure, GCP - Frame logout as making cleanup explicit and comprehensive - Maintain existing detailed documentation structure * Move problem statement to dedicated section in logout docs - Keep intro concise and focused on what the command does - Add 'The Problem' section below intro - Explains credential sprawl and lack of easy logout in cloud tooling - Positions Atmos logout as the solution * [autofix.ci] apply automated fixes * Fix AWS auth tests to use forked homedir package - Tests were calling Reset() on mitchellh/go-homedir - Production code uses our forked pkg/config/homedir - This caused cache to not be cleared properly between test runs - Update both setup_test.go and files_test.go to use our fork - Tests now pass reliably when run multiple times - Add comprehensive Reset() tests to homedir package * Add lint rule to forbid mitchellh/go-homedir imports - Add forbidigo rule to prevent importing github.com/mitchellh/go-homedir - Direct developers to use our forked pkg/config/homedir instead - Prevents future test flakiness from using wrong homedir package - Config verified with golangci-lint config verify * Fix blog post formatting and homedir test flakiness Blog post fixes (2025-10-17-auth-logout-feature.md): - Add shell language specifiers to 7 code blocks (lines 44, 71, 92, 113, 132, 153, 213) - Add missing periods to list items (lines 182, 311, 320) Homedir test fixes (pkg/config/homedir/homedir_test.go): - Add Reset() calls to TestDir and TestExpand to prevent cross-test interference - Fix TestExpand expected path construction (remove trailing separator) - Tests now pass reliably when run multiple times --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
2 participants
what
telemetry.IsCI()checks in authentication logic with aisInteractive()function that checks for TTY availability.pkg/telemetry/ci.goto require bothJENKINS_URLandBUILD_IDto be present for Jenkins CI detection, preventing false positives when onlyJENKINS_URLis set.pkg/telemetry/ci.gofor better visibility into CI detection.aws.AnonymousCredentials{}when loading config for SSO, preventing hangs on default credential providers.why
telemetry.IsCI()was used for runtime behavior decisions (e.g., showing interactive prompts). This is incorrect as telemetry functions should not dictate application behavior. The change separates these concerns by usingisInteractive()for runtime decisions and improving CI detection accuracy.JENKINS_URLenvironment variable was being set bybuild-harnessby default, leading to incorrect Jenkins CI detection in environments that were not actual Jenkins CI. Requiring bothJENKINS_URLandBUILD_IDfor Jenkins detection resolves this false positive.isInteractive()check ensures prompts are only shown when a TTY is available. Explicitly providingaws.AnonymousCredentials{}for SSO config loading prevents the AWS SDK from attempting to find credentials from other sources that might hang.False Jenkins Detection
JENKINS_URL=https://localhost/buildByToken/buildWithParametersby defaultJENKINS_URLexistence → false positives in any project using build-harnessJENKINS_URLANDBUILD_ID(what real Jenkins sets)atmos auth loginin make targetsPre-commit Build Issues
make custom-gclonce, then commits work without rebuildingreferences
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores