Skip to content

Update homedir README with fork details#1673

Merged
aknysh merged 1 commit intomainfrom
osterman/homedir-fork-docs
Oct 20, 2025
Merged

Update homedir README with fork details#1673
aknysh merged 1 commit intomainfrom
osterman/homedir-fork-docs

Conversation

@osterman
Copy link
Member

what

  • Appended a detailed section to pkg/config/homedir/README.md describing the "Atmos Fork Enhancements".
  • This new section explains the fork's prioritization of environment variables for test compatibility with t.Setenv().
  • It also details cache management strategies, including disabling caching (homedir.DisableCache = true) and resetting the cache (homedir.Reset()).
  • Provides code examples for using these features in Go tests.

why

  • To clearly document the specific enhancements made in Atmos's vendored fork of the mitchellh/go-homedir package.
  • To provide users, particularly those writing Go tests, with clear instructions on how to leverage the improved environment variable support and cache management for better testability.
  • The original mitchellh/go-homedir package is deprecated, and this fork is maintained to support these specific testing requirements.

references

  • closes #279

@osterman osterman requested review from a team as code owners October 20, 2025 03:17
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between d2dd4a0 and ee5eb9b.

📒 Files selected for processing (1)
  • pkg/config/homedir/README.md (1 hunks)
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch osterman/homedir-fork-docs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the size/s Small size PR label Oct 20, 2025
@osterman osterman added the no-release Do not create a new release (wait for additional code changes) label Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.30%. Comparing base (d2dd4a0) to head (ee5eb9b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
unittests 66.30% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

osterman added a commit that referenced this pull request Oct 20, 2025
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
@aknysh aknysh merged commit 126306a into main Oct 20, 2025
61 of 62 checks passed
@aknysh aknysh deleted the osterman/homedir-fork-docs branch October 20, 2025 03:55
aknysh pushed a commit that referenced this pull request Oct 22, 2025
* 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>
@github-actions
Copy link

These changes were released in v1.196.0-rc.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release Do not create a new release (wait for additional code changes) size/s Small size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants