feat(DEV-2781): Notify Not Running in a Git Repo #990
Conversation
milldr
added
minor
📝 WalkthroughWalkthroughThe pull request introduces Git repository context validation for the Atmos CLI tool. Two new functions, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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 (2)
cmd/cmd_utils.go (2)
612-618: Consider enhancing Git command execution robustness.The Git repository check could be more robust with these improvements:
- Add a timeout to prevent hanging
- Preserve stderr for debugging
- Consider checking if Git is installed
Here's a suggested implementation:
func isGitRepository() bool { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - cmd := exec.Command("git", "rev-parse", "--is-inside-work-tree") - cmd.Stderr = nil - err := cmd.Run() + cmd := exec.CommandContext(ctx, "git", "rev-parse", "--is-inside-work-tree") + err := cmd.Run() + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + return false + } + // Git might not be installed or other system error + u.LogTrace(atmosConfig, fmt.Sprintf("Git check failed: %v", err)) + return false + } return err == nil }
620-631: Add documentation for environment variables.The environment variable check is well-implemented, but it would be helpful to add documentation explaining:
- The purpose of
ATMOS_BASE_PATHandATMOS_CLI_CONFIG_PATH- When users should consider using these variables instead of Git
Add this documentation above the function:
+// checkGitAndEnvVars validates the execution environment for Atmos. +// It skips the Git repository check if either of these environment variables are set: +// - ATMOS_BASE_PATH: Override the base path for Atmos configuration +// - ATMOS_CLI_CONFIG_PATH: Specify a custom path to the Atmos CLI config file func checkGitAndEnvVars() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(2 hunks)cmd/root.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 (1)
cmd/root.go (1)
112-115: LGTM! Well-placed Git repository check.The Git repository validation is strategically placed after config initialization and before processing custom commands.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/cmd_utils.go (2)
614-647: Consider parameterizing the timeout value and improving error handling.The implementation is solid, but could be enhanced in a few ways:
- Extract the timeout duration as a constant
- Pass
atmosConfigas a parameter instead of using a global variable- Check the actual output of the git command for better accuracy
-func isGitRepository() bool { +const gitCheckTimeout = 3 * time.Second + +func isGitRepository(config schema.AtmosConfiguration) bool { // Create command with timeout context - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), gitCheckTimeout) defer cancel() cmd := exec.CommandContext(ctx, "git", "rev-parse", "--is-inside-work-tree") + var stdout bytes.Buffer var stderr bytes.Buffer + cmd.Stdout = &stdout cmd.Stderr = &stderr // Run the command err := cmd.Run() // Check for timeout if ctx.Err() == context.DeadlineExceeded { - u.LogTrace(atmosConfig, "git check timed out after 3 seconds") + u.LogTrace(config, fmt.Sprintf("git check timed out after %v", gitCheckTimeout)) return false } // Check if git is not installed if errors.Is(err, exec.ErrNotFound) { - u.LogTrace(atmosConfig, "git is not installed") + u.LogTrace(config, "git is not installed") return false } if err != nil { - u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v (stderr: %s)", err, stderr.String())) + u.LogTrace(config, fmt.Sprintf("git check failed: %v (stderr: %s)", err, stderr.String())) return false } - return true + // Check actual output + return strings.TrimSpace(stdout.String()) == "true" }
649-660: Enhance function name and improve warning message.The function implementation is clean but could be improved:
- The function name could be more specific since it primarily checks for git repository
- The warning message could provide more context about using
ATMOS_BASE_PATHorATMOS_CLI_CONFIG_PATH- Consider passing
atmosConfigas a parameter-func checkGitAndEnvVars() { +func checkGitRepositoryOrEnvVars(config schema.AtmosConfiguration) { // Skip check if either env var is set if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { return } // Check if we're in a git repo - if !isGitRepository() { - u.LogWarning(atmosConfig, "You're not inside a git repository. Atmos feels lonely outside - bring it home!") + if !isGitRepository(config) { + u.LogWarning(config, "Not running in a git repository. Set ATMOS_BASE_PATH or ATMOS_CLI_CONFIG_PATH to specify an alternative base path.") } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cmd_utils.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/cmd_utils.go (1)
620-620: Verify git command execution security.The git command execution is secure as it uses specific arguments, but let's verify there are no other git commands in the codebase that might need similar protection.
✅ Verification successful
Git command execution is properly secured 💪
The implementation follows security best practices:
- Uses CommandContext for timeout control
- Has hardcoded arguments preventing injection
- Is the only git command execution in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for git command executions in the codebase rg -n "exec\.Command.*git" -A 2Length of output: 208
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/cmd_utils.go (3)
612-627: Add function documentation.Consider adding a detailed function documentation to explain the purpose and behavior.
+// isGitRepository determines if the current directory is within a Git repository. +// It uses go-git to detect the repository and returns true if found, false otherwise. +// Any non-repository errors are logged at trace level. func isGitRepository() bool {
619-624: Make error logging more specific.The current error logging could be more descriptive about what failed during the Git repository check.
if err != nil { if err != git.ErrRepositoryNotExists { - u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) + u.LogTrace(atmosConfig, fmt.Sprintf("git repository detection failed: %v", err)) } return false }
629-640: Enhance function documentation and user guidance.The function would benefit from detailed documentation and a more helpful warning message.
+// checkGitAndEnvVars verifies if Atmos is running within a Git repository context. +// It skips the check if either ATMOS_BASE_PATH or ATMOS_CLI_CONFIG_PATH environment +// variables are set, as these override the default Git-based workflow. func checkGitAndEnvVars() { // Skip check if either env var is set if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { return } // Check if we're in a git repo if !isGitRepository() { - u.LogWarning(atmosConfig, "You're not inside a git repository. Atmos feels lonely outside - bring it home!") + u.LogWarning(atmosConfig, "Not running in a Git repository. Atmos works best within a Git context.\n"+ + "Consider:\n"+ + "1. Running from a Git repository, or\n"+ + "2. Setting ATMOS_BASE_PATH environment variable to specify the working directory") } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cmd_utils.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/cmd_utils.go (1)
22-22: LGTM! Using native go-git implementation.Good choice using the native go-git library as recommended.
|
Add test case with |
|
Can we move the check lower down.
|
Yes but to do so I have to move inside the But I think that should be fine, since if atmos fails to find atmos.yaml, then we always trigger that function. If we dont, then atmos.yaml is valid and git is likely valid too
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/cmd_utils.go (1)
614-633: 🛠️ Refactor suggestionConsider using go-git for more robust repository detection.
The current implementation only checks for a
.gitdirectory in the current directory. Usinggo-gitwould provide more reliable repository detection by:
- Validating repository integrity
- Supporting repository detection in parent directories
- Following Git's repository discovery rules
Example implementation using go-git:
-func isGitRepository() bool { - // Get current working directory - currentDir, err := os.Getwd() - if err != nil { - u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) - return false - } - - // Check if .git directory exists directly in the current directory - _, err = os.Stat(filepath.Join(currentDir, ".git")) - if err != nil { - if !os.IsNotExist(err) { - u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) - } - return false - } - - return true -} +import "github.com/go-git/go-git/v5" + +func isGitRepository() bool { + // Get current working directory + currentDir, err := os.Getwd() + if err != nil { + u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) + return false + } + + // Attempt to open the repository + _, err = git.PlainOpenWithOptions(currentDir, &git.PlainOpenOptions{ + DetectDotGit: true, + }) + if err != nil { + if err != git.ErrRepositoryNotExists { + u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) + } + return false + } + + return true +}
🧹 Nitpick comments (2)
cmd/cmd_utils.go (1)
635-646: Improve function name and warning message clarity.The function name
checkGitAndEnvVarsimplies checking both Git and environment variables, but it's primarily a Git check with environment variable bypass.Consider these improvements:
-func checkGitAndEnvVars() { +func checkGitRepositoryContext() { // Skip check if either env var is set if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { return } // Check if we're at the root of a git repo if !isGitRepository() { - u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") + u.LogWarning(atmosConfig, "Not running in a Git repository. Atmos works best within a Git repository for version control. Set ATMOS_BASE_PATH or ATMOS_CLI_CONFIG_PATH to override.\n") } }tests/test-cases/empty-dir.yaml (1)
50-66: Consider adding test cases for environment variable bypass.The test case effectively verifies the warning message when running outside a Git repository. Consider adding these additional test cases:
- name: "run atmos with ATMOS_BASE_PATH set" enabled: true snapshot: true description: "Test that Git check is bypassed when ATMOS_BASE_PATH is set" workdir: "fixtures/scenarios/empty-dir" command: "atmos" args: - version env: ATMOS_BASE_PATH: "/tmp" ATMOS_LOGS_LEVEL: Warning expect: stdout: - "^(?!.*git repository).*$" # Should not contain Git repository warning stderr: - "^$" exit_code: 0 - name: "run atmos with ATMOS_CLI_CONFIG_PATH set" enabled: true snapshot: true description: "Test that Git check is bypassed when ATMOS_CLI_CONFIG_PATH is set" workdir: "fixtures/scenarios/empty-dir" command: "atmos" args: - version env: ATMOS_CLI_CONFIG_PATH: "/tmp/atmos.yaml" ATMOS_LOGS_LEVEL: Warning expect: stdout: - "^(?!.*git repository).*$" # Should not contain Git repository warning stderr: - "^$" exit_code: 0🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
📜 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)tests/test-cases/empty-dir.yaml(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/log-level-validation.yaml
[error] 93-93: no new line character at the end of file
(new-line-at-end-of-file)
tests/test-cases/empty-dir.yaml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
cmd/root.go (1)
Line range hint
461-462: LGTM! Good placement of the Git repository check.The Git repository check is appropriately placed in the initialization process, ensuring early detection and warning.
tests/test-cases/log-level-validation.yaml (1)
Line range hint
1-93: LGTM! No changes needed.The log level validation test cases are comprehensive and don't require modifications for the Git repository check feature.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 93-93: no new line character at the end of file
(new-line-at-end-of-file)
This won't be true for much longer. Atmos will be able to discover the atmos configuration from anywhere within a git repo. |
|
Ok, changing the location is too much scope for this PR. Just keep it in the original location. We'll fix location later. |
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test-cases/empty-dir.yaml (1)
50-66: LGTM! Well-structured test case.The test case effectively validates the warning message when Atmos is run outside a Git repository. The test configuration is complete with:
- Appropriate environment variable setup
- Expected stdout message verification
- Proper exit code check
Add a newline at the end of the file to satisfy YAMLlint.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
cmd/cmd_utils.go (1)
614-633: Consider using go-git for more robust Git repository detection.Instead of checking for the
.gitdirectory directly, consider using thego-gitlibrary for a more robust implementation that can handle various Git repository configurations.Example implementation:
+import "github.com/go-git/go-git/v5" func isGitRepository() bool { // Get current working directory currentDir, err := os.Getwd() if err != nil { u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) return false } - // Check if .git directory exists directly in the current directory - _, err = os.Stat(filepath.Join(currentDir, ".git")) + // Open the repository using go-git + _, err = git.PlainOpen(currentDir) if err != nil { - if !os.IsNotExist(err) { - u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) - } + u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) return false } return true }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(2 hunks)tests/test-cases/empty-dir.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
cmd/cmd_utils.go
[failure] 462-462:
undefined: checkGitAndEnvVars
[failure] 639-639:
cannot use nil as bool value in return statement
[failure] 648-648:
missing return
🪛 GitHub Check: Build (ubuntu-latest, linux)
cmd/cmd_utils.go
[failure] 462-462:
undefined: checkGitAndEnvVars
[failure] 639-639:
cannot use nil as bool value in return statement
[failure] 648-648:
missing return
🪛 YAMLlint (1.35.1)
tests/test-cases/empty-dir.yaml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/cmd_utils.go (1)
614-633: LGTM! Well-implemented Git repository check.The
isGitRepositoryfunction correctly checks for the presence of a.gitdirectory and includes appropriate error handling and logging.
cmd/cmd_utils.go
Outdated
| // Check if we're in a git repo and report a warning if not | ||
| checkGitAndEnvVars() |
There was a problem hiding this comment.
Fix undefined function reference.
The code calls checkGitAndEnvVars() but this function is not defined. This is causing a build failure.
Add the missing function:
+// checkGitAndEnvVars verifies Git repository and environment variables
+func checkGitAndEnvVars() {
+ verifyInsideGitRepo()
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 462-462:
undefined: checkGitAndEnvVars
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 462-462:
undefined: checkGitAndEnvVars
cmd/cmd_utils.go
Outdated
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | ||
| func verifyInsideGitRepo() bool { | ||
| // Skip check if either env var is set | ||
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | ||
| return nil | ||
| } | ||
|
|
||
| // Check if we're at the root of a git repo | ||
| if !isGitRepository() { | ||
| u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | ||
| return false | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix return statements and logic in verifyInsideGitRepo.
The function has several issues:
- Returns
nilinstead of a bool value - Has an unreachable
return truestatement - Missing a return statement at the end
Apply this fix:
func verifyInsideGitRepo() bool {
// Skip check if either env var is set
if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" {
- return nil
+ return true
}
// Check if we're at the root of a git repo
if !isGitRepository() {
u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n")
return false
- return true
}
+ return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | |
| func verifyInsideGitRepo() bool { | |
| // Skip check if either env var is set | |
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | |
| return nil | |
| } | |
| // Check if we're at the root of a git repo | |
| if !isGitRepository() { | |
| u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | |
| return false | |
| return true | |
| } | |
| } | |
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | |
| func verifyInsideGitRepo() bool { | |
| // Skip check if either env var is set | |
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | |
| return true | |
| } | |
| // Check if we're at the root of a git repo | |
| if !isGitRepository() { | |
| u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | |
| return false | |
| } | |
| return true | |
| } |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 639-639:
cannot use nil as bool value in return statement
[failure] 648-648:
missing return
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 639-639:
cannot use nil as bool value in return statement
[failure] 648-648:
missing return
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/cmd_utils.go (2)
612-631: Consider using go-git for robust Git repository detection.The current implementation only checks for the
.gitdirectory existence. While simple, this approach might miss edge cases like Git worktrees or submodules.Consider using go-git for more robust Git repository detection:
+import "github.com/go-git/go-git/v5" func isGitRepository() bool { - // Get current working directory - currentDir, err := os.Getwd() - if err != nil { - u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) - return false - } - - // Check if .git directory exists directly in the current directory - _, err = os.Stat(filepath.Join(currentDir, ".git")) - if err != nil { - if !os.IsNotExist(err) { - u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) - } - return false - } - return true + _, err := git.PlainOpen(".") + if err != nil { + u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) + return false + } + return true }
461-463: Consider adding error handling for Git check failure.While the Git check is integrated well, we should handle potential errors gracefully.
Consider adding error handling:
- verifyInsideGitRepo() + if !verifyInsideGitRepo() { + u.LogDebug(atmosConfig, "Proceeding without Git repository context") + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(8 hunks)cmd/root.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` provides user feedback by showing error messages, command suggestions, and valid subcommands before terminating the program with `os.Exit(1)`. It never returns to the caller, making error handling unnecessary for calls to this function.
Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/root.go:172-178
Timestamp: 2025-01-18T15:18:35.475Z
Learning: The `showUsageAndExit` function in `cmd/cmd_utils.go` terminates the program with `os.Exit(1)` and never returns to the caller, making error handling unnecessary for calls to this function.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/cmd_utils.go (1)
633-647: LGTM! Clear environment variable handling.The function correctly prioritizes environment variables over Git repository checks, providing flexibility in configuration.
Let's verify the environment variable usage across the codebase:
✅ Verification successful
Environment variable handling is rock solid! 💪
The environment variables are used consistently throughout the codebase, with ATMOS_BASE_PATH determining the root path for components/stacks and ATMOS_CLI_CONFIG_PATH locating the configuration file. The function correctly prioritizes these environment variables over Git repository checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ATMOS_BASE_PATH and ATMOS_CLI_CONFIG_PATH usage echo "Checking ATMOS_BASE_PATH usage:" rg "ATMOS_BASE_PATH" -A 2 echo -e "\nChecking ATMOS_CLI_CONFIG_PATH usage:" rg "ATMOS_CLI_CONFIG_PATH" -A 2Length of output: 31721
Perhaps we should hold off on this until then. Since we should change this to verify whether or not were in a git repo at all, not just if were at the root of the repo |
|
💥 This pull request now has conflicts. Could you fix it @milldr? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cmd_utils.go(2 hunks)tests/test-cases/empty-dir.yaml(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
cmd/cmd_utils.go
[failure] 617-617:
too many arguments in call to u.LogTrace
[failure] 625-625:
too many arguments in call to u.LogTrace
[failure] 642-642:
too many arguments in call to u.LogWarning
🪛 GitHub Check: Build (ubuntu-latest, linux)
cmd/cmd_utils.go
[failure] 617-617:
too many arguments in call to u.LogTrace
[failure] 625-625:
too many arguments in call to u.LogTrace
[failure] 642-642:
too many arguments in call to u.LogWarning
🪛 YAMLlint (1.35.1)
tests/test-cases/empty-dir.yaml
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
tests/test-cases/log-level-validation.yaml
[error] 93-93: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/cmd_utils.go (1)
461-462: LGTM!Good placement of the Git repository check before config validation. This provides early feedback to users about the missing Git context.
tests/test-cases/empty-dir.yaml (1)
52-68: LGTM! Well-structured test case.The test case effectively validates:
- Warning level logging
- Git repository warning message
- Expected exit code
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
tests/test-cases/log-level-validation.yaml (1)
Line range hint
1-93: LGTM! Comprehensive test coverage.The test suite thoroughly validates log level handling through:
- Configuration file
- Environment variables
- Command line flags
- Invalid and valid scenarios
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 93-93: no new line character at the end of file
(new-line-at-end-of-file)
cmd/cmd_utils.go
Outdated
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | ||
| func verifyInsideGitRepo() bool { | ||
| // Skip check if either env var is set | ||
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | ||
| return true | ||
| } | ||
|
|
||
| // Check if we're at the root of a git repo | ||
| if !isGitRepository() { | ||
| u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Fix LogWarning call argument.
The LogWarning call has incorrect arguments. Remove the atmosConfig argument:
- u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n")
+ u.LogWarning("You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | |
| func verifyInsideGitRepo() bool { | |
| // Skip check if either env var is set | |
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | |
| return true | |
| } | |
| // Check if we're at the root of a git repo | |
| if !isGitRepository() { | |
| u.LogWarning(atmosConfig, "You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | |
| return false | |
| } | |
| return true | |
| } | |
| // verifyInsideGitRepo checks if we're at the root of a git repo, if required env vars are not set | |
| func verifyInsideGitRepo() bool { | |
| // Skip check if either env var is set | |
| if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { | |
| return true | |
| } | |
| // Check if we're at the root of a git repo | |
| if !isGitRepository() { | |
| u.LogWarning("You're not at the root of a git repository. Atmos feels lonely outside - bring it home!\n") | |
| return false | |
| } | |
| return true | |
| } |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 642-642:
too many arguments in call to u.LogWarning
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 642-642:
too many arguments in call to u.LogWarning
cmd/cmd_utils.go
Outdated
| // isGitRepository checks if the current directory is the root of a git repository | ||
| func isGitRepository() bool { | ||
| // Get current working directory | ||
| currentDir, err := os.Getwd() | ||
| if err != nil { | ||
| u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) | ||
| return false | ||
| } | ||
|
|
||
| // Check if .git directory exists directly in the current directory | ||
| _, err = os.Stat(filepath.Join(currentDir, ".git")) | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { | ||
| u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using go-git for more robust Git repository detection.
The current implementation only checks for a .git directory in the current path. Using go-git would provide more reliable Git repository detection and support finding the repository root from subdirectories.
Additionally, fix the LogTrace calls which have incorrect arguments:
- u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err))
+ u.LogTrace(fmt.Sprintf("failed to get current directory: %v", err))- u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err))
+ u.LogTrace(fmt.Sprintf("git check failed: %v", err))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // isGitRepository checks if the current directory is the root of a git repository | |
| func isGitRepository() bool { | |
| // Get current working directory | |
| currentDir, err := os.Getwd() | |
| if err != nil { | |
| u.LogTrace(atmosConfig, fmt.Sprintf("failed to get current directory: %v", err)) | |
| return false | |
| } | |
| // Check if .git directory exists directly in the current directory | |
| _, err = os.Stat(filepath.Join(currentDir, ".git")) | |
| if err != nil { | |
| if !os.IsNotExist(err) { | |
| u.LogTrace(atmosConfig, fmt.Sprintf("git check failed: %v", err)) | |
| } | |
| return false | |
| } | |
| return true | |
| } | |
| // isGitRepository checks if the current directory is the root of a git repository | |
| func isGitRepository() bool { | |
| // Get current working directory | |
| currentDir, err := os.Getwd() | |
| if err != nil { | |
| u.LogTrace(fmt.Sprintf("failed to get current directory: %v", err)) | |
| return false | |
| } | |
| // Check if .git directory exists directly in the current directory | |
| _, err = os.Stat(filepath.Join(currentDir, ".git")) | |
| if err != nil { | |
| if !os.IsNotExist(err) { | |
| u.LogTrace(fmt.Sprintf("git check failed: %v", err)) | |
| } | |
| return false | |
| } | |
| return true | |
| } |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 617-617:
too many arguments in call to u.LogTrace
[failure] 625-625:
too many arguments in call to u.LogTrace
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 617-617:
too many arguments in call to u.LogTrace
[failure] 625-625:
too many arguments in call to u.LogTrace
The intent here was not to check if we are in the root of the git repo, but anywhere inside of a git repo. |
Reverted to check for git repo at all or not. But now the problem is the tests, since they're always inside a git repo |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test-cases/empty-dir.yaml (1)
68-68: Add Newline at End of File
YAML linting indicates that the file is missing a newline at the end. Please add a newline character at the end of the file to comply with formatting standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
cmd/cmd_utils.go (1)
628-641: verifyInsideGitRepo Function Review
TheverifyInsideGitRepofunction correctly bypasses the Git repository check if eitherATMOS_BASE_PATHorATMOS_CLI_CONFIG_PATHis set, and logs a warning when the current directory isn’t a Git repository. One suggestion for further clarity: if the return value isn’t used by the caller (only the side effect matters), consider renaming the function to something likewarnIfNotInGitRepoor explicitly using its return value in the caller to drive subsequent logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(3 hunks)tests/test-cases/empty-dir.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/empty-dir.yaml
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
tests/test-cases/empty-dir.yaml (1)
51-68: New Test Case for Git Repo Warning
The new test case "atmos warns if not in git repo" is clear and aligns with the PR objective. It checks that the Atmos command warns appropriately when run outside a Git repository. One thing to keep in mind is that tests are often executed within a Git repo, so please confirm that the specified workdir ("fixtures/scenarios/empty-dir") truly simulates a non-Git environment.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 68-68: no new line character at the end of file
(new-line-at-end-of-file)
cmd/cmd_utils.go (2)
612-626: isGitRepository Function Implementation
The implementation ofisGitRepositoryleverages go-git effectively to check for a Git repository by using theDetectDotGitoption. Logging a trace message for errors (except for the expectedgit.ErrRepositoryNotExists) helps with debugging. The approach is concise and clear.
462-464: Usage of verifyInsideGitRepo in printMessageForMissingAtmosConfig
WithinprintMessageForMissingAtmosConfig, callingverifyInsideGitRepo()solely for its side effects (i.e. logging the warning) works as intended. Still, to improve clarity and maintainability, consider either using the boolean result (to conditionally trigger additional behavior) or renaming the function to reflect that its primary role is to notify the user rather than act as a strict predicate.
Just set workdir to / |
what
why
references
Summary by CodeRabbit
New Features
Tests