Conversation
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
==========================================
+ Coverage 66.29% 66.30% +0.01%
==========================================
Files 350 350
Lines 39674 39674
==========================================
+ Hits 26301 26307 +6
+ Misses 11367 11361 -6
Partials 2006 2006
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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 * [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>
|
These changes were released in v1.196.0-rc.0. |
what
pkg/config/homedir/README.mddescribing the "Atmos Fork Enhancements".t.Setenv().homedir.DisableCache = true) and resetting the cache (homedir.Reset()).why
mitchellh/go-homedirpackage.mitchellh/go-homedirpackage is deprecated, and this fork is maintained to support these specific testing requirements.references
closes #279