Ensure Logs Level is Validated in All Subcommands, Ensure Version Check Uses GITHUB_TOKEN
#957
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications to improve logging and error handling in the Atmos CLI. Key changes include the addition of debug logging statements during version checks, enhancements to error messages related to log level validation, and updates to the GitHub release checking process. The handling of logging configurations has shifted from environment variables to command line arguments, and various test cases have been updated to reflect these changes. Overall, the updates aim to enhance the clarity and robustness of the Atmos CLI without altering its core functionality. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (1)
cmd/cmd_utils.go (1)
498-512: Fix typo in error message while maintaining good debug logging.The added debug logging enhances observability, but there's a grammatical error in the error message.
Fix the typo in the error message:
-u.LogWarning(atmosConfig, fmt.Sprintf("Unable not load cache: %s", err)) +u.LogWarning(atmosConfig, fmt.Sprintf("Unable to load cache: %s", err))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/cmd_utils.go(2 hunks)cmd/root.go(1 hunks)cmd/version.go(3 hunks)pkg/config/utils.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [lint] quick-start-advanced
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Docker Lint
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: Summary
🔇 Additional comments (7)
cmd/version.go (3)
26-40: Well-structured initialization of log level configuration!The code properly validates the log level from command line flags before initializing the configuration, following good error handling practices.
54-75: Consider the impact of cache updates during version checks.While the added debug logging improves observability, be cautious about using
CheckForAtmosUpdateAndPrintMessagein this context as it updates the cache timestamp, which might not be desired here.Consider separating the version check logic from cache updates to maintain better control over when the cache is modified.
81-82: Good addition of debug logging!The debug message clearly indicates that updates are being checked from the cache, improving observability.
cmd/root.go (1)
65-70: Excellent improvement to error handling!The code now intelligently suppresses unnecessary warnings during help requests while maintaining proper error handling for other scenarios. This enhances the user experience.
cmd/cmd_utils.go (1)
535-536: Good addition of status logging!The debug message clearly indicates when Atmos is up to date, improving observability.
pkg/config/utils.go (2)
364-364: Excellent improvement to error messaging!The error message now clearly indicates both the invalid value and the valid options, making it easier for users to correct their input.
489-489: Great consistency in error messaging!The error message for command line flags matches the style used for environment variables, maintaining a consistent user experience.
Cerebrovinny
force-pushed
the
logs-improvements-part2
branch
from
6c00f9d to
59b899d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/utils/github_utils.go (1)
34-35: Consider using authenticated client for better rate limits.The change to use an unauthenticated client might lead to GitHub API rate limiting. Consider using the existing
newGitHubClientfunction which handles authentication.-client := github.NewClient(nil) +client := newGitHubClient(context.Background())cmd/version.go (1)
56-65: Simplify error handling logic.The error handling contains redundant checks and could be simplified.
- latestReleaseTag, err := u.GetLatestGitHubRepoRelease(atmosConfig, "cloudposse", "atmos") - if err == nil && latestReleaseTag != "" { - if err != nil { - u.LogWarning(atmosConfig, fmt.Sprintf("Failed to check for updates: %v", err)) - return - } - if latestReleaseTag == "" { - u.LogWarning(atmosConfig, "No release information available") - return - } + latestReleaseTag, err := u.GetLatestGitHubRepoRelease(atmosConfig, "cloudposse", "atmos") + if err != nil { + u.LogWarning(atmosConfig, fmt.Sprintf("Failed to check for updates: %v", err)) + return + } + if latestReleaseTag == "" { + u.LogWarning(atmosConfig, "No release information available") + return + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/cmd_utils.go(2 hunks)cmd/root.go(1 hunks)cmd/version.go(4 hunks)pkg/config/utils.go(2 hunks)pkg/utils/github_utils.go(2 hunks)pkg/utils/log_utils.go(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/root.go
- cmd/cmd_utils.go
- pkg/config/utils.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/utils/log_utils.go (1)
82-82: LGTM! Improved debug message clarity.Adding the "[DEBUG]" prefix makes it easier to identify debug level messages in logs.
tests/test-cases/log-level-validation.yaml (1)
34-34: LGTM! Test case updated to match new error message format.The test case now properly validates the improved error message that includes the environment variable context.
cmd/version.go (3)
26-40: LGTM! Proper initialization of logging configuration.The initialization of atmosConfig with log level from flags is well-structured and includes proper error handling.
69-71: LGTM! Clear debug logging.The debug logging statements provide good visibility into the version comparison process.
82-83: LGTM! Added debug logging for cache check.Good addition of debug logging before checking for updates from cache.
|
LGTM, tested locally. Just address the suggestions and we're good to merge. |
osterman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/README.md (1)
95-95: Enhance documentation for the newcleanoption.The new
cleanoption is a valuable addition for maintaining test environment consistency. However, its documentation could be more comprehensive.Consider adding a detailed explanation after the option, similar to other documented fields:
clean: true # Whether or not to remove untracked files from workdir + # Use this option to ensure a clean test environment by removing any + # untracked files that might affect test results. This is particularly + # useful for tests that generate temporary files or artifacts.pkg/config/cache.go (2)
58-58: LGTM! Consider enhancing the logging context.The debug log provides good visibility into cache initialization. However, consider passing the actual configuration context instead of an empty
AtmosConfiguration{}struct for more comprehensive logging.-u.LogDebug(schema.AtmosConfiguration{}, fmt.Sprintf("Cache file not found at %s", cacheFile)) +u.LogDebug(atmosConfig, fmt.Sprintf("Cache file not found at %s", cacheFile))
70-70: LGTM! Consider enhancing the logging context.The debug log confirms successful cache loading. However, consider passing the actual configuration context instead of an empty
AtmosConfiguration{}struct for more comprehensive logging.-u.LogDebug(schema.AtmosConfiguration{}, fmt.Sprintf("Loaded cache from %s", cacheFile)) +u.LogDebug(atmosConfig, fmt.Sprintf("Loaded cache from %s", cacheFile))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks)pkg/config/cache.go(2 hunks)tests/README.md(1 hunks)tests/cli_test.go(5 hunks)tests/fixtures/scenarios/empty-dir/.atmos/cache.yaml(1 hunks)tests/fixtures/scenarios/empty-dir/.gitignore(0 hunks)tests/snapshots/TestCLICommands_check_atmos_--help_has_new_version_available.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir#01.stdout.golden(1 hunks)tests/test-cases/help-and-usage.yaml(7 hunks)
💤 Files with no reviewable changes (1)
- tests/fixtures/scenarios/empty-dir/.gitignore
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/scenarios/empty-dir/.atmos/cache.yaml
- tests/snapshots/TestCLICommands_check_atmos_--help_has_new_version_available.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir#01.stdout.golden
- tests/cli_test.go
- tests/test-cases/help-and-usage.yaml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
.gitignore (1)
44-46: Well documented change, warrior! Let's verify the impact.The comments clearly explain why we're tracking these files. This is excellent documentation that will help future maintainers understand the reasoning.
Let's check what files this will now track:
✅ Verification successful
Tracking these test fixtures is safe and beneficial, warrior! 🛡️
The change is well-justified as all tracked files are:
- Small text-based configuration files (largest is 34KB)
- Essential for testing infrastructure configurations
- Well-organized in a clear directory structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check what files will now be tracked under tests/fixtures # that weren't tracked before # List all files under tests/fixtures fd . tests/fixtures -t f # Check if any large files might be unexpectedly tracked fd . tests/fixtures -t f -x du -h {} | sort -hr | head -n 5Length of output: 18664
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/config/cache.go (1)
Line range hint
83-110: Consider consolidating the save functions for better maintainability.There are two similar functions (
SaveCacheandSaveCache2) with the main difference being file locking inSaveCache2. Consider consolidating these into a single, more robust function.Here's a suggested approach:
-func SaveCache2(cfg CacheConfig) error { +func SaveCache(cfg CacheConfig) error { _, cacheFile, err := GetCacheFilePath() if err != nil { return err } return withCacheFileLock(cacheFile, func() error { v := viper.New() v.Set("last_checked", cfg.LastChecked) if err := v.WriteConfigAs(cacheFile); err != nil { return errors.Wrap(err, "failed to write cache file") } return nil }) } -func SaveCache(cfg CacheConfig) error { - _, cacheFile, err := GetCacheFilePath() - if err != nil { - return err - } - - v := viper.New() - v.Set("last_checked", cfg.LastChecked) - if err := v.WriteConfigAs(cacheFile); err != nil { - return errors.Wrap(err, "failed to write cache file") - } - return nil -}
🧹 Nitpick comments (3)
tests/cli_test.go (2)
737-778: Consider enhancing error handling and logging.The
cleanDirectoryfunction is well-structured, but consider these improvements:
- Add debug logging for Git operations
- Handle symlinks explicitly
- Add a safety check for the working directory path
func cleanDirectory(t *testing.T, workdir string) error { + // Validate working directory + if !filepath.IsAbs(workdir) { + return fmt.Errorf("working directory must be absolute: %q", workdir) + } + + t.Logf("Finding Git repository root from: %q", workdir) // Find the root of the Git repository repoRoot, err := findGitRepoRoot(workdir) if err != nil { return fmt.Errorf("failed to locate git repository from %q: %w", workdir, err) } + t.Logf("Found Git repository root: %q", repoRoot) // Rest of the function... for file, statusEntry := range status { if statusEntry.Worktree == git.Untracked { fullPath := filepath.Join(repoRoot, file) + // Handle symlinks + resolvedPath, err := filepath.EvalSymlinks(fullPath) + if err != nil { + t.Logf("Warning: Failed to resolve symlink %q: %v", fullPath, err) + continue + } if strings.HasPrefix(resolvedPath, workdir) { t.Logf("Removing untracked file: %q\n", fullPath) if err := os.RemoveAll(fullPath); err != nil { return fmt.Errorf("failed to remove %q: %w", fullPath, err) } } } }
780-801: Add input validation and improve error messages.The
findGitRepoRootfunction is well-focused, but consider adding input validation:func findGitRepoRoot(path string) (string, error) { + // Validate input path + if path == "" { + return "", fmt.Errorf("path cannot be empty") + } + + // Ensure path exists + if _, err := os.Stat(path); err != nil { + return "", fmt.Errorf("path does not exist or is not accessible: %w", err) + } + // Open the Git repository starting from the given path repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{DetectDotGit: true}) if err != nil { - return "", fmt.Errorf("failed to find git repository: %w", err) + return "", fmt.Errorf("failed to find git repository from path %q: %w", path, err) }pkg/config/cache.go (1)
Line range hint
25-38: Solid function enhancement, but let's make it even better!The function now provides more context by returning the cache directory separately. However, we could improve the error handling by adding validation for empty cache directory paths.
Consider adding this validation:
func GetCacheFilePath() (string, string, error) { xdgCacheHome := os.Getenv("XDG_CACHE_HOME") var cacheDir string if xdgCacheHome == "" { cacheDir = filepath.Join(".", ".atmos") } else { cacheDir = filepath.Join(xdgCacheHome, "atmos") } + + if strings.TrimSpace(cacheDir) == "" { + return "", "", errors.New("cache directory path cannot be empty") + } if err := os.MkdirAll(cacheDir, 0755); err != nil { return "", "", errors.Wrap(err, fmt.Sprintf("error creating cache directory: %s", cacheDir)) } return cacheDir, filepath.Join(cacheDir, "cache.yaml"), nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/cmd_utils.go(2 hunks)pkg/config/cache.go(4 hunks)tests/cli_test.go(6 hunks)tests/test-cases/help-and-usage.yaml(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cmd_utils.go
- tests/test-cases/help-and-usage.yaml
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (5)
tests/cli_test.go (2)
49-59: LGTM! Well-documented struct fields.The
TestCasestruct fields are well-documented with clear descriptions. The newCleanfield is properly integrated with the existing fields.
332-362: LGTM! Robust test environment setup.The changes improve test isolation and cleanup:
- Creates isolated test environment with temporary directory
- Sets up XDG environment variables correctly
- Resolves working directory to absolute path
- Integrates directory cleaning when enabled
pkg/config/cache.go (3)
20-21: Strong addition, warrior! The CacheConfig struct is now more robust.The addition of
CacheFileandCacheDirfields improves cache location tracking and management.
52-63: Excellent debug logging enhancement, champion!The addition of detailed debug logging and proper initialization of cache fields will make troubleshooting much easier.
66-79: Battle-tested error handling improvements!The enhanced error messages and debug logging provide clear visibility into cache-related issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test-cases/validate-editorconfig.yaml (1)
12-13: Consider moving update notifications to stdout/stderr.The "Update available!" message in the
diffsection might not be the most intuitive place for version notifications. Consider moving these to stdout or stderr to better align with CLI conventions.expect: - diff: - - "Update available!" stdout: + - "Update available!" - "No errors found"Also applies to: 29-30
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden(1 hunks)tests/test-cases/validate-editorconfig.yaml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden
- tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test-cases/metadata.yaml (1)
68-72: Well-structured test expectations, commander!The stdout patterns effectively verify both the initialization and planning phases of Terraform. Consider adding more specific patterns to validate the actual plan output.
Add an additional pattern to verify plan details:
stdout: - "Terraform has been successfully initialized" - "Terraform used the selected providers to generate the following execution" + - "Plan:" # Verify plan summary is present stderr: - "^$"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test-cases/metadata.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/test-cases/metadata.yaml (1)
32-35: Strong work on the output validation patterns, warrior!The regex pattern
\d+for resource counts makes the test robust against varying numbers of resources. The empty stderr check"^$"ensures clean execution without warnings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
58-58: Add a comment explaining the timeout value.For better maintainability, consider documenting why 15 minutes was chosen as the timeout value.
+# 15m timeout accounts for GitHub API operations and extended logging during tests go test $(TEST) -v $(TESTARGS) -timeout 15m
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
Makefile (1)
58-58: LGTM! Let's verify if tests actually need this extended timeout.The increase to 15 minutes is reasonable for acceptance tests, especially with the added logging and GitHub API calls. However, let's verify which test cases benefit from this extension.
Run this script to identify slow test cases:
what
cleanflag to remove untracked files before running tests in workdirEvidence:
wrong/invalid flag passed:
Env variable invalid:
why
Improved error messages
Better output clarity when theres an issue on the ATMOS_LOGS_LEVEL as env variable or when it's passed as a flag
references
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Bug Fixes
Documentation
Chores
.gitignorefile.These changes focus on enhancing the Atmos CLI's logging, error handling, and user experience.