feat(DEV-2781): Notify Not Running in a Git Repo #990
Conversation
📝 WalkthroughWalkthroughA Git repository presence check has been added to the Atmos CLI utility code. Two new functions, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AtmosCLI
participant GitRepoChecker
User->>AtmosCLI: Run command
AtmosCLI->>GitRepoChecker: verifyInsideGitRepo()
alt ATMOS_BASE_PATH/CLI_BASE_PATH not set
GitRepoChecker->>GitRepoChecker: isGitRepository()
alt Not a Git repo
GitRepoChecker->>AtmosCLI: Log warning
end
end
AtmosCLI->>User: Continue with config validation and command execution
Assessment against linked issues
Suggested reviewers
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. 🪧 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
|
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. |
…posse/atmos into DEV-2781/warning-when-not-git-repo
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/cmd_utils.go (1)
691-704: Consider using Viper for environment variables.The function implements the check correctly and provides a good user experience with the warning message. However, according to project guidelines, you should use Viper for environment variables instead of direct
os.Getenvcalls.func verifyInsideGitRepo() bool { // Skip check if either env var is set - if os.Getenv("ATMOS_BASE_PATH") != "" || os.Getenv("ATMOS_CLI_CONFIG_PATH") != "" { + if viper.GetString("ATMOS_BASE_PATH") != "" || viper.GetString("ATMOS_CLI_CONFIG_PATH") != "" { return true }Don't forget to add the import:
import ( + "github.com/spf13/viper"🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 694-694: use of
os.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"(forbidigo)
🪛 GitHub Check: golangci-lint
[failure] 694-694:
use ofos.Getenvforbidden because "Useviper.BindEnvfor new environment variables instead ofos.Getenv"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cmd_utils.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🧬 Code Graph Analysis (1)
cmd/cmd_utils.go (1)
internal/exec/shell_utils.go (2)
ExecuteShell(108-130)ExecuteShellAndReturnOutput(133-162)
🪛 GitHub Check: golangci-lint
cmd/cmd_utils.go
[failure] 26-26:
Comment should end in a period
[failure] 694-694:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
🪛 golangci-lint (1.64.8)
cmd/cmd_utils.go
[error] 694-694: use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
(forbidigo)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/cmd 9E88 _utils.go (5)
12-12: Good job on adding necessary imports.The imports for
github.com/charmbracelet/logandgithub.com/go-git/go-git/v5are correctly structured. The log import follows the project's convention of using the aliaslog.Also applies to: 23-23
188-188: Good refactoring using the new constant.Using the
currentDirPathconstant instead of string literals improves maintainability and consistency.Also applies to: 408-408, 444-444
531-533: Good integration of repository check.Adding the Git repository verification at this point in the flow is appropriate, as it allows early notification to users before they encounter configuration issues.
676-689: Well-implemented Git repository detection.The implementation correctly uses the go-git library with proper error handling. The debug logging for unexpected errors is a good practice.
26-27:❌ Incorrect review comment
Add period to comment for consistency.
The constant declaration comment should end with a period to match the code style guidelines.
-// Define a constant for the dot string that appears multiple times +// Define a constant for the dot string that appears multiple times. const currentDirPath = "."You can verify the style consistency with:
🏁 Script executed:
#!/bin/bash # Check if other comments in the codebase end with periods grep -r "^// " --include="*.go" | grep -v "\.$" | head -n 10Length of output: 921
Trailing period consistency across comments
On inspection, most comments incmd/*.godon’t end with a period, so adding one here would be inconsistent with the existing style. Unless we adopt a project‐wide rule (e.g., via a linter) to enforce periods at sentence ends, please disregard this one‐off change or apply it uniformly across all files.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 26-26:
Comment should end in a period
|
final screenshot: |
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 (1)
cmd/cmd_utils.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/cmd_utils.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-02-21T20:56:20.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
🪛 golangci-lint (1.64.8)
cmd/cmd_utils.go
[error] 694-694: use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
(forbidigo)
🪛 GitHub Check: golangci-lint
cmd/cmd_utils.go
[failure] 694-694:
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/cmd_utils.go (5)
12-12: Package imports look good.The
charmbracelet/logpackage is correctly imported with the aliaslogas per project guidelines, and thego-gitpackage is added to support Git repository detection.Also applies to: 23-23
26-27: Good constant extraction.Creating a constant for the current directory path improves maintainability and reduces the chance of errors. This is a solid refactoring.
187-187: Clean constant usage.Replaced string literals with the new
currentDirPathconstant, creating more consistent and maintainable code.Also applies to: 408-408, 444-444
676-689: Solid Git repository detection implementation.The
isGitRepositoryfunction appropriately uses go-git to check if the current directory is inside a Git repository. The error handling correctly differentiates between expected repository-not-found errors and unexpected failures, with proper debug logging.
532-532: Effective warning integration.Integrating the Git repository check early in the
printMessageForMissingAtmosConfigfunction provides users with clear feedback when Atmos is run outside its intended environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/load.go (1)
104-106: Environment variable bindings look solid.Good addition of the environment variable bindings for base path and CLI config path. This properly enables the warning feature when Atmos is run outside a Git repository while providing alternatives for users.
Just noting that
ATMOS_CLI_CONFIG_PATHis also used inreadEnvAmosConfigPath(line 210) for a specific purpose. While this doesn't create issues, consider adding a comment in both places to clarify the relationship between these two uses of the same environment variable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cmd_utils.go(7 hunks)pkg/config/load.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/cmd_utils.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
==========================================
+ Coverage 30.23% 30.27% +0.03%
==========================================
Files 204 204
Lines 22858 22877 +19
==========================================
+ Hits 6911 6925 +14
- Misses 14908 14912 +4
- Partials 1039 1040 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/cmd_utils_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
cmd/cmd_utils_test.go (6)
1-8: Imports look good, but consider if context is needed.The imports are clean and focused. However, given the PR's objective to verify Git repo presence, you might need the
contextpackage in the future for timeout handling if Git operations take too long.
10-16: Good test setup with proper cleanup.The temporary directory creation and cleanup logic is solid. The defer statement ensures proper resource cleanup even if the test fails.
18-22: Directory state management looks solid.Saving the current directory to restore it later is a good practice to ensure the test doesn't alter the environment permanently.
49-62: Test execution looks clean and follows best practices.The subtest pattern with proper setup and clear assertions is perfect. Each test case is isolated and reports errors clearly.
64-68: Proper directory restoration at the end of the test.This ensures test isolation and prevents side effects.
24-47:✅ Verification successful
Test cases are well structured, but assumptions need to be validated.
The table-driven test approach works well here. However, the test assumes the current working directory is inside a Git repository. This could cause test failures in CI environments that don't clone with Git history.
🏁 Script executed:
#!/bin/bash # Check if the current directory is actually a Git repository if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then echo "Current directory is inside a Git repository (test assumption is valid)" else echo "WARNING: Current directory is NOT inside a Git repository (test may fail)" fiLength of output: 190
Assumption validated: current directory is inside a Git repo
git rev-parse --is-inside-work-treeconfirms our test environment is a Git work tree. CI systems clone with the.gitdirectory by default, so these table‑driven tests should pass without modification. No action required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/test-cases/empty-dir.yaml (6)
46-63: Consider a more robust fixture for a non-Git directory
Usingworkdir: "/"to simulate running outside a Git repo may not be reliable across CI or local environments. I recommend adding an explicit fixture directory (e.g.,fixtures/scenarios/no-git) without a.gitfolder, or using a generated temp dir, to ensure consistent test behavior.
64-66: Remove trailing whitespace in comment
Line 64 has trailing spaces, triggering a YAML lint error. Please trim the extra spaces at the end of this comment.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: trailing spaces
(trailing-spaces)
67-70: Fix grammar in test name/description
The keyword “doesnt” is missing an apostrophe. To be consistent and grammatically correct, please use “doesn’t” in the test name and description.
67-86: Add coverage forATMOS_CLI_BASE_PATH
The PR supports bothATMOS_BASE_PATHandATMOS_CLI_BASE_PATHfor specifying an alternate base path. Currently we only testATMOS_BASE_PATH. Consider adding a third test case that exercisesATMOS_CLI_BASE_PATHto ensure that alias works as expected. For example:+ - name: atmos doesn’t warn if not in git repo with config via ATMOS_CLI_BASE_PATH + enabled: true + snapshot: true + description: "Test that atmos doesn’t warn when ATMOS_CLI_BASE_PATH is set outside a Git repo" + workdir: "/" + command: "atmos" + args: + - list + - stacks + env: + ATMOS_LOGS_LEVEL: Warning + ATMOS_CLI_BASE_PATH: "fixtures/scenarios/complete" + expect: + diff: [] + stdout: + - !not ".*You're not inside a git repository\\. Atmos feels lonely outside.*" + stderr: + - "^$" + exit_code: 0
87-106: Fix grammar in test name/description
The test uses “doesnt” without an apostrophe. Please replace with “doesn’t” to maintain consistency and clarity.
107-121: Ensure stdout assertion for missing config
In this failure scenario (no atmos config), the test checks exit code and absence of the Git warning, but doesn’t assert that the “atmos.yaml CLI config file was not found.” message is printed. Consider adding astdoutexpectation to verify the missing‑config prompt.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_in_git_repo_with_atmos_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_in_git_repo_with_no_atmos_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_not_in_git_repo_with_atmos_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_warns_if_not_in_git_repo_with_no_atmos_config.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_warns_if_not_in_git_repo_with_no_atmos_config.stdout.golden(1 hunks)tests/test-cases/empty-dir.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- tests/snapshots/TestCLICommands_atmos_warns_if_not_in_git_repo_with_no_atmos_config.stderr.golden
- tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_not_in_git_repo_with_atmos_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_in_git_repo_with_no_atmos_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_warns_if_not_in_git_repo_ 2684 with_no_atmos_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_doesnt_warn_if_in_git_repo_with_atmos_config.stdout.golden
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/empty-dir.yaml
[error] 64-64: trailing spaces
(trailing-spaces)
⏰ 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 (3)
tests/test-cases/empty-dir.yaml (3)
64-64: Remove trailing whitespace
Line 64 has trailing spaces, which may fail YAML linting. Please remove them.- # We need to test from outside a git repo. + # We need to test from outside a git repo.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: trailing spaces
(trailing-spaces)
102-103: Simplify regex quoting for readability
The!notpattern uses a double-quoted string with escaped backslashes. Switching to a single-quoted scalar reduces the need for double escapes and improves clarity:- - !not ".*You're not inside a git repository\\. Atmos feels lonely outside - bring it home!.*" + - !not '.*You''re not inside a git repository\. Atmos feels lonely outside - bring it home!.*'
108-112: Unify test naming for consistency
The nameatmos doesnt warn if run inside of a git repo with no atmos configis a bit verbose and slightly different from other cases. For consistency, consider:- - name: atmos doesnt warn if run inside of a git repo with no atmos config + - name: atmos doesnt warn if in git repo with no atmos config
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test-cases/empty-dir.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
tests/test-cases/empty-dir.yaml
[error] 64-64: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
tests/test-cases/empty-dir.yaml (2)
46-63: Verify the log level used in tests
The tests setATMOS_LOGS_LEVEL: Warning. Please confirm that the CLI accepts this exact value (many implementations useWarn). A mismatch here could cause the warning check to be skipped.
67-78: ValidateATMOS_BASE_PATHresolution
This test usesworkdir: "../../"and then setsATMOS_BASE_PATH: "./atmos/tests/fixtures/scenarios/complete". Ensure that this relative path actually points to the fixtures directory in both your local environment and CI—otherwise the config lookup will fail.
what
why
references
Summary by CodeRabbit