Feature: Added config file feature detection and schema validation #154
Conversation
…nd new `ahoy doctor` command Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
|
Warning Rate limit exceeded@ocean has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a contributor and README badge updates; introduces path expansion and a hidden simulate-version flag; changes getConfig signature; adds config init and config validate commands with comprehensive validation/reporting, tests, testdata, Makefile targets, and an indirect dependency bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Ahoy CLI
participant FS as Filesystem
participant Net as HTTP
User->>CLI: ahoy config init [--force] [URL]
CLI->>FS: Check for .ahoy.yml
alt exists and not --force
CLI->>User: Prompt overwrite?
User-->>CLI: yes/no
opt no
CLI-->>User: Abort without changes
end
end
CLI->>Net: GET example config (URL or default)
Net-->>CLI: Response body / error
CLI->>FS: Write .ahoy.yml
CLI-->>User: Success or error message
sequenceDiagram
participant User
participant CLI as Ahoy CLI
participant FS as Filesystem
User->>CLI: ahoy config validate
CLI->>FS: Locate .ahoy.yml
alt not found
CLI-->>User: Warn and exit
else found
CLI->>CLI: Load config (getConfig with skipValidation)
CLI->>CLI: ValidateConfig (versions, imports, envs)
CLI->>FS: Check import/env file existence (expandPath)
CLI-->>User: PrintConfigReport with recommendations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
v2/flag.go (1)
9-30: Consider validating the simulated version format.The implementation looks good for testing purposes. However, consider adding validation to ensure the simulated version follows a valid format (e.g., semantic versioning) to prevent potential issues in the validation logic.
You could add version format validation when this flag is used, similar to:
if simulateVersion != "" && !isValidVersion(simulateVersion) { logger("fatal", "Invalid version format for --simulate-version") }README.md (1)
226-226: Fix minor formatting issues in sponsor section.The sponsor image update looks good, but there are a few formatting issues to address:
- Add alt text to the image for accessibility
- Consider using asterisk (*) instead of dash (-) for consistency with markdown list style
-- [<img src="https://avatars.githubusercontent.com/u/378794?s=160&v=4" width="160px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com) +* [<img src="https://avatars.githubusercontent.com/u/378794?s=160&v=4" alt="Alex Skrypnyk - DrevOps" width="160px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com)v2/ahoy.go (1)
47-47: Global state for validation control.The
skipValidationvariable is used appropriately to prevent validation during the doctor command execution. However, using global state can be problematic in concurrent scenarios.Consider passing validation control through function parameters or context instead of global state for better maintainability and thread safety.
v2/validation.go (1)
32-32: Clarify the comment about multiple_env_files supportThe comment "Actually supported since the beginning via StringArray" might be confusing. Consider clarifying whether this is a legacy feature that always worked or if the version requirement is still relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.all-contributorsrc(1 hunks)README.md(3 hunks)v2/ahoy.go(5 hunks)v2/doctor.go(1 hunks)v2/doctor_test.go(1 hunks)v2/flag.go(2 hunks)v2/tests/missing-cmd.bats(1 hunks)v2/validation.go(1 hunks)
🧬 Code Graph Analysis (3)
v2/flag.go (1)
v2/vendor/github.com/urfave/cli/flag_string.go (1)
StringFlag(6-16)
v2/validation.go (1)
v2/ahoy.go (2)
Config(21-27)Command(31-40)
v2/doctor_test.go (3)
v2/doctor.go (4)
RunDoctor(38-78)ImportFileStatus(30-35)DoctorResult(10-20)EnvFileStatus(23-27)v2/validation.go (2)
ValidationResult(24-27)ValidationIssue(11-21)v2/ahoy.go (2)
Config(21-27)Command(31-40)
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
🧰 Additional context used
🧬 Code Graph Analysis (3)
v2/flag.go (1)
v2/vendor/github.com/urfave/cli/flag_string.go (1)
StringFlag(6-16)
v2/validation.go (1)
v2/ahoy.go (2)
Config(21-27)Command(31-40)
v2/doctor_test.go (3)
v2/doctor.go (4)
RunDoctor(38-78)ImportFileStatus(30-35)DoctorResult(10-20)EnvFileStatus(23-27)v2/validation.go (2)
ValidationResult(24-27)ValidationIssue(11-21)v2/ahoy.go (2)
Config(21-27)Command(31-40)
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (27)
v2/tests/missing-cmd.bats (1)
24-24: LGTM! Error message update aligns with enhanced validation.The updated error message is more specific and consistent with the improved error handling introduced in the validation framework.
README.md (2)
3-3: Contributor count correctly updated.The badge update reflects the addition of Jonathan Nagy as a contributor.
8-8: CI badge updated to GitHub Actions.Good to see the migration to GitHub Actions for CI/CD.
.all-contributorsrc (1)
162-171: New contributor entry properly added.The addition of Jonathan Nagy follows the correct format and includes appropriate contribution types.
v2/ahoy.go (4)
180-184: Improved error visibility for import failures.Good enhancement to log warnings when import files fail to load, making debugging easier while maintaining backward compatibility.
480-482: Doctor command properly added to defaults.The command is correctly added only if it doesn't already exist, preventing duplicates.
142-156: Validation functions confirmed; validation logic approved.Both
ValidateConfigandPrintValidationIssuesare implemented inv2/validation.go(lines 87–124 and 275–320), so the validation block inv2/ahoy.gooperates correctly. No further changes needed.
341-380: Approved: Enhanced error messaging with version support verified
- Confirmed
GetAhoyVersion,VersionSupports, and theFeatureSupportmap are implemented inv2/validation.go.- Missing-file detection and user guidance are clear and actionable.
- Version checks correctly gate optional imports and point users to upgrade when needed.
- “ahoy doctor” reference provides an easy troubleshooting path.
No further changes required.
v2/doctor_test.go (11)
9-36: Test implementation looks good!The test properly covers the scenario when a config file doesn't exist, checking all relevant fields and the expected recommendation.
38-77: Good test coverage for invalid YAML scenario!The test correctly simulates invalid YAML syntax and verifies the expected behavior.
79-115: Well-structured test for valid configuration!The test properly validates all expected fields for a valid configuration file.
117-171: Excellent test coverage for environment file detection!The test properly validates the detection of existing and missing environment files at both global and command levels.
173-230: Comprehensive test for import file handling!The test effectively covers both required and optional import files, properly validating their existence and optional status.
232-258: Good unit test for version mismatch recommendations!The test properly validates that version mismatch errors generate the appropriate upgrade recommendation.
260-287: Well-designed test for missing import file recommendations!The test correctly validates that missing required import files trigger the appropriate recommendation.
289-315: Good coverage for missing environment file recommendations!The test properly validates the recommendation generation for missing environment files.
317-343: Effective test for newer feature recommendations!The test correctly handles warning-level version mismatches and generates appropriate upgrade suggestions.
345-367: Nice test for the happy path!The test properly validates that a clean configuration generates an appropriate success message.
369-556: Thorough test coverage for helper functions and data structures!The remaining tests effectively validate:
- Environment and import file checking logic
- All struct fields are properly accessible and functional
- Helper functions work correctly with relative paths
Great job on comprehensive test coverage!
v2/doctor.go (5)
9-35: Well-structured data types for diagnostic results!The structs provide a clean and comprehensive representation of the diagnostic information.
37-78: Clean implementation of the diagnostic workflow!The function follows a logical progression through all diagnostic checks with proper early returns for error conditions.
80-108: Proper implementation of environment file checking!The function correctly handles both global and command-specific environment files with appropriate path resolution.
134-195: Comprehensive recommendation generation logic!The function effectively analyzes all diagnostic results and generates appropriate, actionable recommendations for each scenario.
197-321: Excellent user-friendly diagnostic report formatting!The function provides clear, visually appealing output with appropriate emoji indicators and well-structured information. Great attention to user experience!
v2/validation.go (3)
37-47: Good version handling with testing support!The function properly handles version simulation for testing and provides sensible defaults.
86-124: Well-structured configuration validation!The function provides comprehensive validation with proper error tracking and clear separation of concerns.
274-320: Clear and informative validation issue reporting!The function provides excellent user feedback with proper severity indication and helpful suggestions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
3-4: Keep the contributors badge in sync with the tableThe badge count was manually bumped to 14.
To avoid future drift, regenerate the badge with theall-contributorsCLI (npx all-contributors-cli generate) instead of hand-editing, or add the new contributor through the CLI (add).
That will update both the badge and the list consistently.
226-226: Add missing alt text to comply with MD045The sponsor image lacks
alttext, triggering markdown-lint. Add analtattribute so screen-readers and automated checks are happy.- [<img src="https://avatars.githubusercontent.com/u/378794?s=100&v=4" width="100px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com) + [<img src="https://avatars.githubusercontent.com/u/378794?s=100&v=4" width="100px;" alt="Alex Skrypnyk avatar"><br />Alex Skrypnyk – DrevOps](https://drevops.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (1)
README.md (1)
8-8: Confirm the workflow file path in the badge URLThe badge now points to
actions/workflows/build_and_test.yml.
If the workflow file is renamed or moved (common when cleaning up CI), the badge will silently break. Double-check that the file exists on the default branch and, if required, append?branch=mainso the badge always reflects the status ofmain.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
226-226: Missingaltattribute and inconsistent bullet marker.
markdownlintflags both issues. Fix with:- - [<img src="https://avatars.githubusercontent.com/u/378794?s=100&v=4" width="100px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com) + * [<img src="https://avatars.githubusercontent.com/u/378794?s=100&v=4" width="100px;" alt="Alex Skrypnyk avatar"><br />Alex Skrypnyk - DrevOps](https://drevops.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
226-226: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
226-226: Images should have alternate text (alt text)
(MD045, no-alt-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (3)
README.md (3)
3-3: Badge count/style change looks good.The switch to
flatand increment to 14 accurately reflect the new contributor.
8-8: Verify that the referenced workflow exists.Please confirm that
actions/workflows/build_and_test.ymlis present onmain(and in forks) so the badge doesn’t render as “no status”.
250-250: Table extension LGTM.New contributor cell follows the established structure and keeps the 7-column layout intact.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/doctor.go (1)
82-109: Consider handling tilde expansion for environment filesWhile import files support tilde expansion (lines 121-125), environment files don't have the same support. Consider adding consistent path expansion for environment files.
// Check global environment files for _, envPath := range config.Env { - fullPath := filepath.Join(configDir, envPath) + fullPath := envPath + if !strings.HasPrefix(envPath, "/") && !strings.HasPrefix(envPath, "~") { + fullPath = filepath.Join(configDir, envPath) + } else if strings.HasPrefix(envPath, "~") { + if home, err := os.UserHomeDir(); err == nil { + fullPath = filepath.Join(home, envPath[2:]) + } + } envFiles = append(envFiles, EnvFileStatus{ Path: envPath, Exists: fileExists(fullPath), Global: true, }) }Apply similar changes to the command-specific environment files loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)v2/doctor.go(1 hunks)v2/validation.go(1 hunks)
🧬 Code Graph Analysis (2)
v2/doctor.go (3)
v2/validation.go (3)
ValidationResult(25-28)GetAhoyVersion(39-48)ValidateConfig(101-138)v2/ahoy.go (2)
Command(31-40)Config(21-27)v2/vendor/github.com/urfave/cli/command.go (1)
Commands(97-97)
v2/validation.go (2)
v2/ahoy.go (2)
Config(21-27)Command(31-40)v2/vendor/github.com/urfave/cli/command.go (1)
Commands(97-97)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/doctor.go (3)
v2/validation.go (3)
ValidationResult(25-28)GetAhoyVersion(39-48)ValidateConfig(101-138)v2/ahoy.go (2)
Command(31-40)Config(21-27)v2/vendor/github.com/urfave/cli/command.go (1)
Commands(97-97)
v2/validation.go (2)
v2/ahoy.go (2)
Config(21-27)Command(31-40)v2/vendor/github.com/urfave/cli/command.go (1)
Commands(97-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (12)
v2/doctor.go (5)
10-36: Well-structured diagnostic typesThe struct definitions are clear and comprehensive, providing all necessary fields for diagnostic reporting.
39-79: Solid diagnostic flow implementationThe function properly handles all diagnostic steps with appropriate early returns for error cases.
111-137: Proper import path handling with tilde expansionThe function correctly handles various path types including tilde expansion, addressing the previous review comments.
139-200: Comprehensive recommendation generationThe function provides helpful, actionable recommendations for various configuration issues.
202-326: Excellent user-friendly diagnostic outputThe report format is clear, visually appealing, and provides comprehensive diagnostic information with appropriate visual indicators.
v2/validation.go (7)
11-36: Well-designed validation structures and feature mappingThe validation structures provide comprehensive fields for detailed issue reporting, and the feature support map clearly defines version requirements.
73-98: Proper semantic version comparison implementedThe version comparison correctly handles semantic versioning by parsing components as integers, properly addressing the previous review comment about string comparison issues.
100-138: Clear validation orchestrationThe main validation function properly orchestrates various validation checks with appropriate error handling.
140-162: Appropriate feature validationThe feature validation correctly checks version compatibility and provides helpful warnings.
164-213: Comprehensive command validationThe function thoroughly validates all aspects of command configuration with appropriate version checks.
215-268: Proper import validation with path expansionThe function correctly handles import validation with proper path expansion including tilde support, addressing the previous review comment.
292-338: Clear validation issue reportingThe function provides well-formatted, user-friendly output with appropriate severity indicators and helpful suggestions.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
|
@ocean Can |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/ahoy.go (1)
47-47: Global state mutation creates race condition risk.The global
skipValidationflag can cause race conditions when multiple Ahoy instances run concurrently. This issue was previously identified but remains unaddressed.Consider using a context-based approach instead:
-skipValidation bool +type ConfigOptions struct { + SkipValidation bool +} // In doctor command: -skipValidation = true -result := RunDoctor(configFile) -skipValidation = false +opts := ConfigOptions{SkipValidation: true} +result := RunDoctorWithOptions(configFile, opts)Also applies to: 490-493
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/ahoy.go(6 hunks)v2/ahoy_test.go(1 hunks)v2/doctor.go(1 hunks)v2/validation.go(1 hunks)
🧬 Code Graph Analysis (1)
v2/validation.go (1)
v2/ahoy.go (2)
Config(21-27)Command(31-40)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/doctor.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/validation.go (1)
v2/ahoy.go (2)
Config(21-27)Command(31-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (7)
v2/ahoy_test.go (1)
238-286: Well-structured and comprehensive path expansion tests.The test coverage for
expandPathis thorough, covering all major scenarios including absolute paths, relative paths, tilde expansion (both Unix and Windows styles), and edge cases. The error handling for home directory lookup failures is appropriately considered.v2/ahoy.go (3)
82-106: Excellent path expansion implementation.The
expandPathfunction properly handles all path types (absolute, tilde, relative) with appropriate error handling for cases where the home directory cannot be determined. This provides consistent path resolution throughout the codebase.
197-208: Good error handling improvements.The addition of warning logs for failed import file loads improves debugging capabilities whilst maintaining backwards compatibility by continuing execution. The consistent use of
expandPathensures uniform path handling.
365-401: Excellent user experience improvements for error diagnostics.The enhanced error messages provide clear, actionable guidance including:
- Identification of missing files
- Multiple solution options
- Version compatibility checks
- Reference to the diagnostic tool
This significantly improves troubleshooting capabilities.
v2/validation.go (3)
73-98: Proper semantic version comparison implemented.The version comparison function correctly handles semantic versioning by:
- Stripping the 'v' prefix
- Splitting version strings into components
- Comparing each numeric part in order
This addresses the previous review comment about string comparison issues.
220-221: Path expansion properly implemented.Both
validateImportandvalidateEnvFilenow use theexpandPathfunction, ensuring consistent path handling including tilde expansion. This addresses the previous review comments about inconsistent path handling.Also applies to: 268-269
100-138: Excellent validation framework implementation.The validation system provides:
- Clear issue categorisation with appropriate severity levels
- Version-aware feature compatibility checking
- Detailed, actionable error messages
- User-friendly output formatting
- Backwards compatibility through appropriate use of warnings
The framework significantly enhances configuration troubleshooting capabilities.
Also applies to: 285-331
|
@AlexSkrypnyk Yep cool, good idea. I didn't want to add a new top level command at all, but that is a good suggestion to use "config" as a reserved word and then we can have sub-commands under it in future. |
|
Also given this a new top level command, it's probably a breaking change that we should release as Ahoy v3 rather than another v2 release 🤔 |
… cross platform compatibility Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
…tructure of "ahoy config ..." as "ahoy config validate" Updated lots of tests, divided commands into separate files. Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
v2/testdata/.env.test (1)
1-2: Add missing newline at end of file.The file lacks a newline character at the end, which can cause issues with some tools and violates POSIX standards for text files.
TEST=value -ANOTHER_VAR=another_value +ANOTHER_VAR=another_value +v2/testdata/missing-api-version.ahoy.yml (1)
1-4: Add missing newline at end of file.The file lacks a newline character at the end, which violates YAML formatting standards and can cause issues with some parsers.
commands: test: description: Test command - cmd: echo "test" + cmd: echo "test" +v2/testdata/wrong-api-version.ahoy.yml (1)
1-5: Add missing newline at end of file.The file lacks a newline character at the end, which violates YAML formatting standards.
ahoyapi: v1 commands: test: description: Test command - cmd: echo "test" + cmd: echo "test" +v2/testdata/with-env-files.ahoy.yml (1)
1-10: Fix formatting: Add newline at end of file.The YAML file is missing a newline character at the end, which violates standard formatting conventions.
env: - .env.command +v2/testdata/with-imports.ahoy.yml (1)
1-12: Fix formatting: Add newline at end of file.The YAML file is missing a newline character at the end, which violates standard formatting conventions.
- another-missing.ahoy.yml optional: true +v2/testdata/newer-features.ahoy.yml (1)
1-13: Fix formatting: Add newline at end of file.The YAML file is missing a newline character at the end, which violates standard formatting conventions.
- optional-import.ahoy.yml optional: true +v2/config_init_test.go (2)
9-26: Consider removing or implementing skipped network tests.This test sets up the environment but then skips execution due to network dependencies. Consider either implementing the test with mocked network calls or removing it entirely if the integration tests provide sufficient coverage.
28-52: Consider removing or implementing skipped network tests.Similar to the previous test, this sets up the environment but skips execution. The setup code provides some value for testing directory handling, but the overall test doesn't contribute to coverage.
v2/config_init.go (1)
72-85: Good error handling pattern but consider graceful degradation.The CLI action function properly constructs the arguments and handles errors by printing to stderr and exiting. However, the direct
os.Exit(1)call makes the function difficult to test.Consider returning the error instead of calling
os.Exit(1)directly:func initCommandAction(c *cli.Context) { + if err := initCommandActionImpl(c); err != nil { + fmt.Fprintln(os.Stderr, "Error:", err) + os.Exit(1) + } +} + +func initCommandActionImpl(c *cli.Context) error { args := InitArgs{ Force: c.Bool("force"), } if len(c.Args()) > 0 { args.URL = c.Args()[0] } - if err := RunConfigInit(args); err != nil { - fmt.Fprintln(os.Stderr, "Error:", err) - os.Exit(1) - } + return RunConfigInit(args) }This would make the function more testable while maintaining the same CLI behaviour.
v2/config_validation.go (3)
11-22: Consider using constants for Type and Severity valuesTo improve type safety and prevent typos, consider defining constants for the
TypeandSeverityfield values instead of using raw strings.+// ValidationIssueType constants +const ( + IssueTypeVersionMismatch = "version_mismatch" + IssueTypeUnknownField = "unknown_field" + IssueTypeMissingFile = "missing_file" + IssueTypeSyntaxError = "syntax_error" +) + +// ValidationSeverity constants +const ( + SeverityError = "error" + SeverityWarning = "warning" + SeverityInfo = "info" +) + // ValidationIssue represents a configuration validation problem type ValidationIssue struct { - Type string // "version_mismatch", "unknown_field", "missing_file", "syntax_error" - Severity string // "error", "warning", "info" + Type string + Severity string Message string
35-35: Update version comment when releasingThe comment "This version we're implementing" should be updated or removed when v2.6.0 is actually released.
129-135: Optimise error checking by setting HasError immediatelyInstead of looping through all issues at the end, set
HasErrorwhen adding error-level issues.Consider setting
result.HasError = trueimmediately when adding error-level issues in lines 117 and within the validation functions, then remove this loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
v2/ahoy.go(8 hunks)v2/ahoy_test.go(2 hunks)v2/config.go(1 hunks)v2/config_init.go(1 hunks)v2/config_init_test.go(1 hunks)v2/config_validation.go(1 hunks)v2/config_validation_test.go(1 hunks)v2/testdata/.env.test(1 hunks)v2/testdata/invalid-yaml.ahoy.yml(1 hunks)v2/testdata/missing-api-version.ahoy.yml(1 hunks)v2/testdata/newer-features.ahoy.yml(1 hunks)v2/testdata/with-env-files.ahoy.yml(1 hunks)v2/testdata/with-imports.ahoy.yml(1 hunks)v2/testdata/wrong-api-version.ahoy.yml(1 hunks)v2/tests/config-init.bats(1 hunks)v2/tests/config-validate.bats(1 hunks)v2/windows_test.go(1 hunks)
🧬 Code Graph Analysis (2)
v2/config_init_test.go (1)
v2/config_init.go (1)
InitArgs(14-17)
v2/config_validation.go (1)
v2/ahoy.go (2)
Config(20-26)Command(30-39)
🪛 GitHub Actions: Build and test
v2/config_init_test.go
[error] 9-92: gofmt formatting check failed. There are whitespace differences and missing newline at end of file. Run 'gofmt -w' to fix code style issues.
🪛 dotenv-linter (3.3.0)
v2/testdata/.env.test
[warning] 2-2: [EndingBlankLine] No blank line at the end of the file
[warning] 2-2: [UnorderedKey] The ANOTHER_VAR key should go before the TEST key
🪛 YAMLlint (1.37.1)
v2/testdata/invalid-yaml.ahoy.yml
[error] 5-5: syntax error: expected ',' or ']', but got ''
(syntax)
v2/testdata/missing-api-version.ahoy.yml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/newer-features.ahoy.yml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/with-env-files.ahoy.yml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/with-imports.ahoy.yml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/wrong-api-version.ahoy.yml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/ahoy_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/config_init_test.go (1)
v2/config_init.go (1)
InitArgs(14-17)
v2/config_validation.go (1)
v2/ahoy.go (2)
Config(20-26)Command(30-39)
🪛 GitHub Actions: Build and test
v2/config_init_test.go
[error] 9-92: gofmt formatting check failed. There are whitespace differences and missing newline at end of file. Run 'gofmt -w' to fix code style issues.
🪛 dotenv-linter (3.3.0)
v2/testdata/.env.test
[warning] 2-2: [EndingBlankLine] No blank line at the end of the file
[warning] 2-2: [UnorderedKey] The ANOTHER_VAR key should go before the TEST key
🪛 YAMLlint (1.37.1)
v2/testdata/invalid-yaml.ahoy.yml
[error] 5-5: syntax error: expected ',' or ']', but got ''
(syntax)
v2/testdata/missing-api-version.ahoy.yml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/newer-features.ahoy.yml
[error] 13-13: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/with-env-files.ahoy.yml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/with-imports.ahoy.yml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
v2/testdata/wrong-api-version.ahoy.yml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (61)
v2/testdata/invalid-yaml.ahoy.yml (1)
1-5: LGTM - intentionally invalid YAML for testing.The invalid YAML syntax with the unclosed array is correctly designed to test YAML parsing error handling in the validation system.
v2/windows_test.go (1)
17-17: LGTM - correctly updated function signature.The
getConfigcall has been properly updated to include the newskipValidationparameter, maintaining the existing test behaviour whilst adapting to the enhanced configuration loading functionality.v2/config.go (1)
9-27: Well-structured validation command implementation.The function properly handles config file discovery, provides helpful user guidance for missing files, and cleanly separates validation execution from reporting. The use of
ValidateOptions{SkipValidation: true}is appropriate for the validate command.v2/config_init_test.go (2)
54-68: Good structural validation of InitArgs.The test effectively validates that the InitArgs struct fields work as expected, providing good coverage of the basic struct functionality.
70-95: Comprehensive testing of fileExists helper.The test covers all relevant scenarios: non-existent files, existing files, and directories. This provides good confidence in the helper function's behaviour.
v2/config_init.go (4)
1-11: Imports look appropriate for the functionality.The imports cover all necessary functionality: file operations, command execution, path handling, and CLI framework integration.
13-17: Well-structured configuration struct.The
InitArgsstruct provides a clean interface for the init command parameters with appropriate field names and types.
19-44: Proper file existence checking and user interaction.The function correctly checks for existing
.ahoy.ymlfiles and provides appropriate user prompts. The logic for handling force mode versus interactive confirmation is well-implemented.
62-69: Clear success messaging.The conditional messaging based on whether a custom URL was provided gives users appropriate feedback about what was downloaded.
v2/config_validation_test.go (19)
1-6: Standard test file structure.Proper package declaration and testing import for a Go test file.
7-34: Well-structured test for non-existent config files.The test properly validates the expected behaviour when a config file doesn't exist, checking both the boolean flags and the presence of appropriate recommendations.
36-61: Good test coverage for invalid YAML handling.The test uses a static test file from
testdata/which is a good practice for reproducible tests. The assertions cover both the existence check and validation results.
63-84: Comprehensive validation of successful config parsing.The test checks multiple aspects of a valid configuration: existence, validity, API version, and that the Ahoy version is populated.
86-122: Thorough testing of environment file detection.The test validates that environment files are properly detected and their status is correctly reported. The approach of building maps to verify expected paths is solid.
124-152: Good coverage of import file functionality.The test properly validates import file detection, including checking the
optionalflag and existence status. The verification thatsimple.ahoy.ymlexists in testdata shows good test data management.
154-180: Effective testing of recommendation generation.The test creates a minimal
ConfigReportwith specific validation issues and verifies that appropriate recommendations are generated. This is a good unit test approach.
182-209: Good test for missing import file scenarios.The test constructs a realistic scenario with missing import files and validates that the correct recommendations are generated.
211-237: Appropriate test for missing environment files.Similar to the import file test, this properly validates environment file recommendation generation.
239-265: Good coverage of version mismatch scenarios.The test validates that newer feature warnings generate appropriate upgrade recommendations.
267-289: Important test for the happy path.Testing the "no issues" scenario ensures that the system provides positive feedback when everything is correct.
291-335: Comprehensive environment file checking test.The test uses a realistic configuration structure and validates both the detection logic and the global/command-specific environment file categorisation.
337-382: Thorough import file checking validation.The test covers both regular and optional import files, ensuring that the optional flag is properly detected and reported.
384-417: Good structural validation of ConfigReport.This test ensures that all expected fields are present and working correctly in the
ConfigReportstruct.
419-438: Proper field validation for EnvFileStatus.The test validates the structure and field accessibility of the
EnvFileStatusstruct.
440-464: Complete field validation for ImportFileStatus.The test ensures all fields of the
ImportFileStatusstruct are accessible and working correctly.
466-484: Important edge case testing.Testing wrong API version scenarios ensures that the validation correctly handles version mismatches and provides appropriate error responses.
486-503: Good coverage of missing API version scenario.This test ensures that configurations without API version specifications are properly detected and handled.
505-525: Validates newer features handling.The test ensures that configurations with newer features are properly parsed and validated, which is important for backwards compatibility.
v2/tests/config-validate.bats (11)
1-15: Proper Bats test setup with good isolation.The setup and teardown functions provide proper test isolation by creating temporary directories and cleaning up afterwards. This prevents tests from interfering with each other.
17-24: Good help output validation.The test properly validates that the help command works and contains expected content. The regex patterns ensure key information is present.
26-32: Appropriate handling of missing config files.The test validates that the command provides helpful guidance when no configuration file exists, including the suggestion to use
ahoy config init.
34-58: Comprehensive validation of successful config processing.The test creates a valid configuration and validates multiple aspects of the output including file detection, API version validation, YAML syntax checking, and overall health assessment.
60-75: Good error detection for invalid YAML.The test creates a configuration with intentionally broken YAML syntax and validates that the error is properly detected and reported.
77-91: Proper API version validation testing.The test validates that missing API version fields are detected and result in appropriate error messages.
93-108: Good coverage of wrong API version scenario.The test ensures that incorrect API versions (like v1) are properly rejected with clear error messages.
110-135: Thorough environment file validation testing.The test creates a realistic scenario with missing environment files and validates that the validation command properly detects and reports on them.
137-170: Comprehensive import file testing.The test covers both existing and missing import files, including optional imports, and validates that the validation output properly reports their status.
172-190: Good complex error scenario testing.The test creates a configuration with multiple issues and validates that the most critical error (API version) is properly reported first.
192-211: Important subdirectory execution test.This test ensures that the validation command works correctly when run from a subdirectory, which is a common real-world scenario.
v2/tests/config-init.bats (8)
1-15: Consistent test infrastructure.The setup and teardown follow the same pattern as the validation tests, providing good consistency and proper isolation.
17-27: Good test of basic init functionality.The test validates that the init command successfully downloads and creates a configuration file with expected content.
29-42: Important backwards compatibility testing.The test ensures that the legacy
ahoy initcommand still works and properly displays the deprecation notice, which is crucial for user migration.
44-60: Realistic user interaction testing.The test simulates user input ('n') to decline overwriting and validates that the original file remains unchanged. This is important for data safety.
62-79: Good validation of overwrite acceptance.The test simulates user agreement ('y') and validates that the file is properly replaced with new content.
81-98: Important force flag testing.The test validates that the
--forceflag bypasses user prompts and overwrites files directly, which is crucial for automation scenarios.
100-107: Pragmatic handling of network-dependent testing.The test acknowledges the challenge of testing network operations in CI environments and focuses on validating the command structure instead.
109-117: Proper help output validation.The test ensures that the help command provides complete information about the init command and its options.
v2/ahoy.go (8)
80-104: Well-implemented path expansion utility.The
expandPathfunction properly handles different path types (absolute, tilde, relative) with appropriate OS compatibility considerations. The logic is clear and the error handling for home directory expansion is appropriate.
126-126: Good simplification of loop condition.Removing the redundant
err == nilcheck makes the loop condition clearer and more focused on the actual termination condition.
141-182: Improved configuration loading with validation control.The addition of the
skipValidationparameter provides better control over when validation occurs. The conditional validation logic and improved error handling (returning errors instead of callingos.Exit) address the previous review comment about error handling patterns.
195-206: Enhanced path handling and error logging.The use of
expandPathfor consistent path resolution and the addition of error logging for failed imports improve the robustness and debuggability of the import system.
363-387: Significantly improved error diagnostics.The enhanced error message for commands with imports provides much more helpful information, including:
- Context about missing import files
- Specific suggestions for resolution
- Version-aware upgrade recommendations
- Reference to the validation command for additional help
This is a substantial improvement in user experience.
388-401: Good version compatibility checking.The validation of optional import support based on the current Ahoy version prevents runtime errors and provides clear upgrade guidance when newer features are used with older versions.
416-468: Well-structured command hierarchy reorganisation.The restructuring introduces a clean
configcommand withinitandvalidatesubcommands while maintaining backwards compatibility with the legacyinitcommand. The deprecation notice helps users migrate to the new structure.
564-564: Consistent parameter passing.The update to pass
falseforskipValidationin the main setup ensures that normal operations include validation while allowing commands likevalidateto skip it when needed.v2/config_validation.go (6)
164-213: Well-structured command validationThe command validation logic is comprehensive and properly handles all command features with appropriate version checks.
215-261: Comprehensive import validation logicThe import validation correctly handles all scenarios with appropriate severity levels and helpful error messages.
460-521: Excellent recommendation generation logicThe function provides comprehensive, actionable recommendations covering all issue types with a positive message when no issues are found.
523-647: Excellent diagnostic report formattingThe report uses clear visual indicators (emojis) and well-structured sections to present comprehensive diagnostic information in a user-friendly format.
384-384: Verified getConfig definition
- The
getConfig(file string, skipValidation bool) (Config, error)function is implemented in v2/ahoy.go (at lines 141–182).- No further changes needed.
329-329: Ignore circular suggestion concernThe
PrintValidationIssuesfunction (v2/config_validation.go) is never invoked by thevalidatesubcommand—instead,validateCommandActioncallsPrintConfigReport. Therefore the advice to runahoy config validateonly appears during other commands and isn’t redundant. You can safely disregard this comment.Likely an incorrect or invalid review comment.
v2/config_validation.go
Outdated
| // GetAhoyVersion returns the current Ahoy version | ||
| func GetAhoyVersion() string { | ||
| // Allow simulation of older versions for testing | ||
| if simulateVersion != "" { | ||
| return simulateVersion | ||
| } | ||
| if version == "" { | ||
| return "development" | ||
| } | ||
| return version | ||
| } | ||
|
|
||
| // GetAhoyVersionForTesting allows overriding version for testing | ||
| func GetAhoyVersionForTesting(testVersion string) string { | ||
| if testVersion != "" { | ||
| return testVersion | ||
| } | ||
| return GetAhoyVersion() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify global variable definitions and consider simplifying test function
The functions reference simulateVersion and version variables that aren't defined in this file. Additionally, GetAhoyVersionForTesting seems redundant.
Consider removing GetAhoyVersionForTesting as GetAhoyVersion already handles test scenarios via simulateVersion.
🏁 Script executed:
#!/bin/bash
# Search for the definition of simulateVersion and version variables
rg -A 2 'var\s+(simulateVersion|version)\s+'Length of output: 137
Define missing simulateVersion and simplify test override
The reference to simulateVersion in GetAhoyVersion will fail to compile—only version is declared in v2/ahoy.go. You should:
• Add a declaration for simulateVersion in v2/ahoy.go, for example:
// ahoy.go
var (
version string
simulateVersion string
)Or remove the simulateVersion branch from GetAhoyVersion if you don’t need runtime simulation.
• Reevaluate GetAhoyVersionForTesting: it merely wraps GetAhoyVersion. If you introduce a setter for simulateVersion, you can remove this function and call GetAhoyVersion directly in tests after setting the global, or vice versa.
Locations to update:
- v2/ahoy.go: declare
simulateVersion(or strip out simulation logic) - v2/config_validation.go (lines 38–56): remove or merge GetAhoyVersionForTesting
🤖 Prompt for AI Agents
In v2/ahoy.go, add a declaration for the variable simulateVersion alongside
version to avoid compilation errors due to its use in v2/config_validation.go.
Then, in v2/config_validation.go lines 38 to 56, either remove the
simulateVersion logic from GetAhoyVersion if runtime simulation is not needed,
or keep it and simplify by removing GetAhoyVersionForTesting entirely, using a
setter function to assign simulateVersion in tests and calling GetAhoyVersion
directly instead.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/ahoy.go (1)
85-87: Simplify absolute path detection logic.The condition checks both
filepath.IsAbs(path)andstrings.HasPrefix(path, "/"), which is redundant. On Windows,filepath.IsAbs()correctly identifiesC:\pathas absolute, butstrings.HasPrefix(path, "/")would be false. The Unix-style prefix check is unnecessary sincefilepath.IsAbs()handles cross-platform absolute path detection.- if filepath.IsAbs(path) || strings.HasPrefix(path, "/") { - // Absolute path (either OS-native or Unix-style), return as-is + if filepath.IsAbs(path) { + // Absolute path, return as-is
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/ahoy.go(8 hunks)v2/config.go(1 hunks)v2/config_init.go(1 hunks)v2/config_init_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v2/config_init_test.go
- v2/config.go
- v2/config_init.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (6)
v2/ahoy.go (6)
126-126: Good simplification of loop termination logic.Removing the redundant
err == nilcheck makes the loop condition clearer and more focused on the actual termination condition (reaching the root directory).
166-180: Excellent improvement to error handling and validation control.The conditional validation logic and proper error return (instead of
os.Exit(1)) addresses previous feedback about inconsistent error handling patterns. This makes the function more testable and follows Go conventions.
195-206: Improved path handling and error resilience.The use of
expandPathensures consistent path resolution, and the enhanced error handling allows graceful degradation when import files fail to load while providing useful diagnostic information.
363-399: Excellent enhancement to import error diagnostics.The detailed error messages with specific solutions and version compatibility checks significantly improve the user experience. The multi-step guidance (create files, use optional imports, upgrade Ahoy) provides clear paths forward when imports fail.
562-562: Consistent with new function signature.Correctly updates the call to match the new
getConfigsignature while maintaining existing validation behavior.
414-466: Verified:initCommandActionandvalidateCommandActionare presentBoth action handlers are defined in the expected files:
initCommandActionin v2/config_init.go (lines 72–85)validateCommandActionin v2/config.go (lines 10–27)No further changes needed.
…gh the code Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
…o built-in HTTP client This means it's now OS-independent and also more secure. Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
v2/config_validation.go (3)
41-41: ** VariablesimulateVersionis not declared**The reference to
simulateVersionwill cause a compilation error as onlyversionis declared in v2/ahoy.go. This issue was previously identified and needs to be resolved.
201-201: ** Warning message remains unclear**The suggestion "This should work but consider upgrading for better support" is still confusing for users. The previous feedback to clarify this message has not been addressed.
461-461: ** Inconsistent path resolution in environment file checking**These functions use
filepath.JoinwhilevalidateEnvFileusesexpandPath. This inconsistency was previously identified and should be fixed for uniform path handling.Also applies to: 472-472
🧹 Nitpick comments (1)
v2/config_validation.go (1)
330-376: ** Consider using consistent output stream for validation messages**The function outputs to
os.Stderrbut the main config report usesos.Stdout. Consider using a consistent approach or making the output stream configurable.-func PrintValidationIssues(result ValidationResult) { +func PrintValidationIssues(result ValidationResult, writer io.Writer) { if len(result.Issues) == 0 { return } - fmt.Fprintf(os.Stderr, "\nConfiguration Validation Issues:\n") + fmt.Fprintf(writer, "\nConfiguration Validation Issues:\n")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/config_validation.go(1 hunks)
🧬 Code Graph Analysis (1)
v2/config_validation.go (1)
v2/ahoy.go (2)
Config(20-26)Command(30-39)
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/config_validation.go (1)
v2/ahoy.go (2)
Config(20-26)Command(30-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (7)
v2/config_validation.go (7)
67-143: ** Excellent improvement to version comparison logic**The version comparison function now properly handles pre-release versions and error cases from
strconv.Atoi, addressing the previous review feedback. The implementation correctly follows semantic versioning rules for pre-release precedence.
266-266: ** Correct use ofexpandPathfor consistent path resolution**Good use of
expandPathhere for handling tilde expansion and relative paths consistently with the validation approach.
145-183: ** Well-structured main validation function**The
ValidateConfigfunction provides a comprehensive validation framework that properly orchestrates different validation checks and aggregates results effectively.
I think adding a new reserved word is not changing API much to have it as a new major version. But leaving this with you to decide. Thank you for working on this |
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
v2/config_validation.go (1)
201-201: Update suggestion text as per previous reviewThe suggestion text could be clearer about why upgrading is recommended.
🧹 Nitpick comments (1)
v2/config_validation.go (1)
35-35: Clarify the comment about schema validation versionThe comment "This version we're implementing" is ambiguous. Consider clarifying it to indicate whether this is the version that introduces schema validation support.
- "schema_validation": "v2.6.0", // This version we're implementing + "schema_validation": "v2.6.0", // Version that introduces schema validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/ahoy.go(11 hunks)v2/config_validation.go(1 hunks)v2/tests/no-ahoy-file.bats(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/ahoy.go (1)
v2/config_validation.go (5)
ValidateConfig(146-183)PrintValidationIssues(331-376)GetAhoyVersion(39-48)VersionSupports(51-63)FeatureSupport(31-36)
v2/config_validation.go (1)
v2/ahoy.go (2)
Config(20-26)Command(30-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (15)
v2/tests/no-ahoy-file.bats (2)
27-28: Tests correctly updated for new command structureThe test assertions properly validate the new deprecation notice for
ahoy init, which aligns with the PR's objective to move this functionality underahoy config init.
35-36: Consistent test updates for --force flagThe test for
ahoy init --forceis consistently updated to check for the same deprecation notice.v2/ahoy.go (7)
81-105: Well-implemented path expansion utilityThe
expandPathfunction correctly handles all path types (absolute, tilde, relative) with proper cross-platform support. The fallback behaviour for failed home directory retrieval is appropriate.
142-182: Improved error handling and validation controlThe refactored
getConfigfunction correctly implements conditional validation and returns errors instead of callingos.Exit(1)directly, addressing the previous review feedback. The verbose mode check appropriately reduces noise for non-error warnings.
196-207: Enhanced error handling for import file loadingGood improvement to log warnings when import files fail to load and continue processing other imports. Using
expandPathensures consistent path resolution across the codebase.
259-260: Consistent path expansion for environment filesGood use of
expandPathfor environment file paths, maintaining consistency with import file handling.
364-400: Excellent error diagnostics for import issuesThe enhanced error messages provide clear, actionable guidance for users encountering import-related issues. The version-aware suggestions and detailed diagnostics significantly improve the user experience.
414-467: Well-structured command refactoring with backward compatibilityThe refactoring successfully implements the new
ahoy configcommand structure withinitandvalidatesubcommands while maintaining backward compatibility through the legacyinitcommand. The deprecation notice appropriately guides users to the new command structure.
51-51: simulateVersion flag integration confirmedThe
--simulate-versionflag is defined in v2/flag.go (Destination: &simulateVersion), so the variable is correctly wired to the CLI. No further changes required here.v2/config_validation.go (6)
38-63: Well-implemented version checking with test supportThe version handling functions correctly support version simulation for testing and handle development versions appropriately.
67-143: Comprehensive semantic version comparisonThe
compareVersionsfunction correctly implements semantic versioning comparison, including proper handling of pre-release versions and parsing errors, addressing the previous review feedback.
145-183: Comprehensive configuration validation logicThe
ValidateConfigfunction provides thorough validation coverage with appropriate error tracking.
411-452: Well-structured validation orchestrationThe
RunConfigValidatefunction effectively orchestrates the validation process with proper error handling and recommendation generation.
454-503: Consistent path expansion in file checking functionsGood to see
expandPathbeing used consistently incheckEnvironmentFilesandcheckImportFiles, addressing the previous review comment about path handling consistency.
568-692: Excellent user-friendly diagnostic outputThe
PrintConfigReportfunction provides clear, visually appealing output with emoji indicators that make it easy to understand the configuration status at a glance. The structured format with sections for issues, files, and recommendations is very helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
5-8: Keep badge alt-text consistent with other imagesThe new “Build and test” badge lacks a descriptive alt-text (
Build and testis OK, but consider adding “GitHub Actions” for clarity and screen-reader users).
Example:[](...)Small accessibility win, zero functional impact.
252-253: Unordered-list style breaks project markdownlint ruleLine 252 begins with
*, whereas the rest of the repo (and markdownlint rule MD004) expects-.
Replace the asterisk with a dash to silence the linter and keep style uniform.-* [<img src="https://avatars.githubusercontent.com/u/378794?s=160&v=4" alt="Alex Skrypnyk - DrevOps" width="160px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com) +- [<img src="https://avatars.githubusercontent.com/u/378794?s=160&v=4" alt="Alex Skrypnyk - DrevOps" width="160px;"><br />Alex Skrypnyk - DrevOps](https://drevops.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
252-252: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (1)
README.md (1)
276-276: Contributor row added – looks goodAvatar URL, alt-text, and contribution icons are all correct; no action needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
v2/ahoy.go (2)
344-366: Avoid env var leakage across command runs (use a local copy per invocation)envVars is captured by the Action closure and appended to; this can accumulate per-command env vars across runs within the same process. Use a per-execution copy.
var cmdArgs []string var cmdEntrypoint []string + // Start with a fresh copy of global env vars per execution. + cmdEnvVars := append([]string{}, envVars...) @@ if len(cmd.Env) > 0 { for _, envPath := range cmd.Env { - cmdEnvFile := expandPath(envPath, AhoyConf.srcDir) - vars := getEnvironmentVars(cmdEnvFile) - if vars != nil { - envVars = append(envVars, vars...) - } + cmdEnvFile := expandPath(envPath, AhoyConf.srcDir) + vars := getEnvironmentVars(cmdEnvFile) + if vars != nil { + cmdEnvVars = append(cmdEnvVars, vars...) + } } } @@ - command.Env = append(command.Environ(), envVars...) + command.Env = append(command.Environ(), cmdEnvVars...)
54-62: Ensure hidden simulate-version flag is fully wired and isolated in testsWe’ve confirmed:
- v2/flag.go defines the hidden “simulate-version” CLI flag, binding to the global
simulateVersion.- v2/config_validation.go’s
GetAhoyVersion()honourssimulateVersionwhen set.Next steps:
- Add or update tests for
GetAhoyVersion()to cover both simulated and real-version paths.- In each test case, explicitly reset the global
simulateVersion(for example in setup/teardown) to prevent cross-test state leakage.
♻️ Duplicate comments (1)
v2/ahoy.go (1)
151-191: Fix wrong filename in API version error; include filename in validation error; consider removing duplicate API check
- Bug: Line 168 references global sourcefile instead of the provided file.
- Suggestion: Include the filename in the returned validation error to aid debugging.
- Optional: The API version check duplicates ValidateConfig; consider relying solely on validation for consistency.
if config.AhoyAPI != "v2" { - err = errors.New("Ahoy only supports API version 'v2', but '" + config.AhoyAPI + "' given in " + sourcefile) + err = errors.New("Ahoy only supports API version 'v2', but '" + config.AhoyAPI + "' given in " + file) return config, err } @@ - if validationResult.HasError { - return config, errors.New("configuration validation failed") - } + if validationResult.HasError { + return config, fmt.Errorf("configuration validation failed for %s", file) + }Note: Great improvement over previous behaviour (no more os.Exit within getConfig); this addresses earlier feedback regarding error handling.
🧹 Nitpick comments (4)
v2/ahoy.go (4)
54-54: Document test-only simulateVersion to prevent misuseAdd a brief comment clarifying this is set via a hidden flag for tests and not for production use.
var version string -var simulateVersion string +// Used by tests via a hidden CLI flag to emulate older Ahoy versions. +// Not intended for production use; leave unset by default. +var simulateVersion string
84-115: Normalise paths with filepath.Clean (minor hardening)Function is correct; normalise outputs to avoid redundant separators and up-levels. Optional: the explicit "/" check is likely redundant on Windows as filepath.IsAbs handles rooted paths.
if filepath.IsAbs(path) { - // OS-native absolute path - return path + // OS-native absolute path + return filepath.Clean(path) } // On Windows, also treat Unix-style paths as absolute for cross-platform compatibility if strings.HasPrefix(path, "/") { // Unix-style absolute path - return path + return filepath.Clean(path) } else if strings.HasPrefix(path, "~") { // Tilde path, expand to home directory if home, err := os.UserHomeDir(); err == nil { // Handle both ~/path and ~\path (Windows) remainder := path[1:] if len(remainder) > 0 && (remainder[0] == '/' || remainder[0] == '\\') { remainder = remainder[1:] } - return filepath.Join(home, remainder) + return filepath.Clean(filepath.Join(home, remainder)) } // If we can't get home dir, return original path - return path + return filepath.Clean(path) } else { // Relative path, join with base directory - return filepath.Join(baseDir, path) + return filepath.Clean(filepath.Join(baseDir, path)) }
428-476: Enforce the “config” reserved top-level command to avoid consumer clashesYou currently only add the default config command if not already present. This doesn’t “reserve” the name—consumer configs defining “config” will silently shadow these tools. Recommend one of:
- Detect and warn/fail on user-defined “config” to preserve the reserved namespace.
- Always register the built-in “config” command first and prevent overrides.
- Document and enforce a set of reserved names at load time (e.g., {"config", "init"}).
This aligns with the PR goal to future-proof and avoid clashes.
468-470: Consider gating the deprecation note behind verbose or send to stderrPrinting a note to stdout can interfere with scripts parsing stdout from init. Consider printing to stderr or only when verbose is enabled.
- fmt.Println("Note: 'ahoy init' is now available as 'ahoy config init'") + fmt.Fprintln(os.Stderr, "Note: 'ahoy init' is now available as 'ahoy config init'")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/ahoy.go(11 hunks)v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
v2/ahoy.go (1)
v2/config_validation.go (5)
ValidateConfig(146-183)PrintValidationIssues(331-376)GetAhoyVersion(39-48)VersionSupports(51-63)FeatureSupport(31-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (5)
v2/ahoy.go (5)
136-146: LGTM: Robust root detection in traversalUsing prevDir to terminate at filesystem root is correct and avoids relying on error state.
205-216: Good: Expand include paths and validate imported configs with graceful degradationImports now use expandPath and run validation; failures are logged as warnings and skipped. This makes imports safer without hard-failing the whole app.
Also applies to: 212-216
268-269: Global env path expansion is correctUsing expandPath against AhoyConf.srcDir improves portability for env files.
377-414: Clear, actionable diagnostics for missing imports and version supportHelpful guidance and version gating for optional_imports. This should reduce user friction when imports are missing or the Ahoy version is outdated.
576-576: LGTM: setupApp now opts into validation during initial config loadUpdated call site matches new signature and intended flow.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/Makefile (1)
76-76: Consider adding the conventional "all" target.The static analysis tool correctly identifies that most Makefiles include an "all" target as a convention. While not required for functionality, it improves consistency with standard Makefile patterns.
Consider adding an "all" target:
+all: build + .PHONY: clean test fmt fmtcheck staticcheck vet gocyclo modernize modernize-check version testdeps cross build_dir default install all
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/Makefile(2 hunks)v2/ahoy.go(13 hunks)v2/config_validation.go(1 hunks)v2/config_validation_test.go(1 hunks)v2/description_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/config_validation_test.go
- v2/config_validation.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
v2/description_test.go (2)
v2/windows_test.go (2)
TestWindowsPathHandling(11-25)TestWindowsConfigPathResolution(27-73)v2/ahoy_test.go (1)
TestGetConfig(163-208)
v2/ahoy.go (1)
v2/config_validation.go (5)
ValidateConfig(146-183)PrintValidationIssues(331-376)GetAhoyVersion(39-48)VersionSupports(51-63)FeatureSupport(31-36)
🪛 checkmake (0.2.2)
v2/Makefile
[warning] 76-76: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (16)
v2/Makefile (2)
45-47: Well-implemented formatting target.The
fmttarget correctly appliesgofmt -wto all Go source files. The implementation using$(foreach ...)is appropriate for this use case.
62-69: Excellent addition of modernization targets.The
modernizeandmodernize-checktargets provide valuable tooling for maintaining modern Go code patterns. The separation between applying fixes (-fix) and preview mode (-test) follows best practices.v2/description_test.go (1)
12-12: Correctly addressed the missing skipValidation parameter.All
getConfigcalls have been properly updated to include the newskipValidationparameter with the appropriatefalsevalue for test scenarios. This resolves the past review comment about missing function arguments.Also applies to: 69-69, 157-157, 202-202
v2/ahoy.go (13)
9-9: Appropriate addition of maps import.The
mapspackage import is correctly added to support themaps.Copyfunctionality used in the custom help printer.
55-55: Well-designed simulation variable for testing.The
simulateVersionvariable provides a clean mechanism for testing version-specific behaviour without requiring actual version changes. This supports the version simulation functionality referenced in the related code snippets.
85-115: Robust path expansion implementation.The
expandPathfunction handles multiple path types correctly:
- Absolute paths (both OS-native and Unix-style for cross-platform compatibility)
- Tilde expansion with proper home directory resolution
- Relative paths joined with base directory
- Graceful fallback for home directory resolution failures
The Windows compatibility considerations are particularly well thought out.
137-137: Simplified loop condition improves readability.The removal of
&& err == nilfrom the loop condition simplifies the logic whilst maintaining the same functional behaviour. The error handling is properly managed elsewhere in the function.
152-152: Function signature update addresses past review.The addition of the
skipValidationparameter togetConfigcorrectly addresses the past review comment about missing arguments in test files.
177-191: Excellent validation integration with proper error handling.The validation logic correctly:
- Skips validation when requested (for diagnostic commands)
- Shows issues only in verbose mode or when errors exist
- Returns an error instead of calling
os.Exit(1)directlyThis addresses the past review comment about inconsistent error handling patterns.
206-217: Improved import handling with proper error management.The enhanced import loading logic:
- Uses the new
expandPathfunction for consistent path resolution- Provides informative warnings for failed imports instead of silent failures
- Continues processing other imports when one fails
This is a significant improvement in robustness.
269-269: Consistent path expansion for environment files.The use of
expandPathfor both global and command-level environment files ensures consistent path resolution across all configuration contexts.Also applies to: 350-350
377-402: Enhanced error reporting for missing imports.The improved error handling for imports provides:
- Detailed context about missing files
- Actionable solutions for users
- Version-aware guidance for feature availability
- Reference to the new
ahoy config validatecommandThis significantly improves the user experience when configuration issues occur.
404-414: Proper version compatibility checking for optional imports.The validation logic correctly checks if the current version supports optional imports and provides clear upgrade guidance when the feature is not available. This prevents silent failures and guides users toward appropriate solutions.
429-481: Well-structured config command implementation.The new command structure:
- Introduces the reserved
configtop-level command as agreed in PR discussions- Maintains backwards compatibility with the existing
initcommand- Provides clear usage descriptions for all subcommands
- Follows proper CLI command organisation patterns
The backwards compatibility notice is a nice touch for user guidance.
577-577: Correct usage of updated getConfig signature.The call to
getConfig(AhoyConf.srcFile, false)correctly uses the new signature with validation enabled for normal application startup.
594-594: Safe map copying implementation.The use of
maps.Copyto merge template functions is a clean and safe approach that avoids potential issues with manual map merging.
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
This is done using the new
ahoy config ...top level command, including theahoy config validatecommand for the config file schema validation and system configuration report.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores