fix: show warning when config is invalid #609
Conversation
WalkthroughThe changes add warning visibility for YAML parsing errors in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds warning messages when the CLI encounters invalid YAML syntax in configuration files, addressing issue #608. Previously, parse errors were silently ignored, making it difficult for users to debug configuration problems.
Key changes:
- Introduced
isDebugMode()function to check debug flags before cobra initialization - Added error handling to distinguish between "config file not found" (silent) and "config parsing error" (warning)
- Updated CHANGELOG to document the new behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/root.go | Implemented warning messages for config parsing errors with debug mode support |
| CHANGELOG.md | Documented the new warning behavior for invalid YAML config files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)cmd/root.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Lints
cmd/root.go
[failure] 69-69:
missing whitespace above this line (invalid statement above if) (wsl_v5)
⏰ 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). (4)
- GitHub Check: Cleanup artifacts
- GitHub Check: Test Release Process
- GitHub Check: Tests
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
CHANGELOG.md (1)
10-11: LGTM! Clear and helpful changelog entry.The entry accurately describes the fix and provides clear guidance to users on how to obtain detailed error messages.
cmd/root.go (2)
21-21: LGTM! Import is correctly used.The
errorspackage is properly imported for theerrors.As()type assertion on line 153.
150-161: LGTM! Error handling logic is correct.The enhanced error handling properly distinguishes between:
- Config file not found (silent continuation, as intended)
- Config file found but failed to parse (warning with appropriate verbosity)
The use of
errors.As()for type checking is correct, andviperInstance.ConfigFileUsed()is safe to call here since we know the file was found (just failed to parse).
| // isDebugMode checks if debug mode is enabled via --debug flag or FGA_DEBUG env var. | ||
| // We are not following cobra's built-in flag checking here because we want to determine | ||
| // debug mode status before cobra parses flags (to control logging during initConfig). | ||
| // The precedence is: | ||
| // 1. Command-line flag --debug | ||
| // 2. Environment variable FGA_DEBUG | ||
| // Other areas in the code should parse the flag using cobra after initialization rather | ||
| // than rely on this function. | ||
| func isDebugMode() bool { | ||
| // Command-line flag takes precedence | ||
| for _, arg := range os.Args { | ||
| if arg == "--debug=true" { | ||
| return true | ||
| } | ||
| if arg == "--debug=false" { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Check environment variable first | ||
| if os.Getenv("FGA_DEBUG") == "true" { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Fix bugs in debug mode detection and correct misleading comment.
The function has several issues:
-
Missing support for
--debugwithout explicit value: Line 99 defines--debugas a boolean flag, so--debug(without=true) should enable debug mode, but this implementation only checks for--debug=trueand--debug=false. -
Misleading comment on line 74: The comment states "Check environment variable first" but the code actually checks the command-line flag first (lines 65-72), then the environment variable.
-
Style issue (linter): Missing whitespace before line 69.
Apply this diff to fix all issues:
func isDebugMode() bool {
// Command-line flag takes precedence
for _, arg := range os.Args {
+ if arg == "--debug" {
+ return true
+ }
if arg == "--debug=true" {
return true
}
+
if arg == "--debug=false" {
return false
}
}
- // Check environment variable first
+ // Fall back to environment variable
if os.Getenv("FGA_DEBUG") == "true" {
return true
}
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // isDebugMode checks if debug mode is enabled via --debug flag or FGA_DEBUG env var. | |
| // We are not following cobra's built-in flag checking here because we want to determine | |
| // debug mode status before cobra parses flags (to control logging during initConfig). | |
| // The precedence is: | |
| // 1. Command-line flag --debug | |
| // 2. Environment variable FGA_DEBUG | |
| // Other areas in the code should parse the flag using cobra after initialization rather | |
| // than rely on this function. | |
| func isDebugMode() bool { | |
| // Command-line flag takes precedence | |
| for _, arg := range os.Args { | |
| if arg == "--debug=true" { | |
| return true | |
| } | |
| if arg == "--debug=false" { | |
| return false | |
| } | |
| } | |
| // Check environment variable first | |
| if os.Getenv("FGA_DEBUG") == "true" { | |
| return true | |
| } | |
| return false | |
| } | |
| // isDebugMode checks if debug mode is enabled via --debug flag or FGA_DEBUG env var. | |
| // We are not following cobra's built-in flag checking here because we want to determine | |
| // debug mode status before cobra parses flags (to control logging during initConfig). | |
| // The precedence is: | |
| // 1. Command-line flag --debug | |
| // 2. Environment variable FGA_DEBUG | |
| // Other areas in the code should parse the flag using cobra after initialization rather | |
| // than rely on this function. | |
| func isDebugMode() bool { | |
| // Command-line flag takes precedence | |
| for _, arg := range os.Args { | |
| if arg == "--debug" { | |
| return true | |
| } | |
| if arg == "--debug=true" { | |
| return true | |
| } | |
| if arg == "--debug=false" { | |
| return false | |
| } | |
| } | |
| // Fall back to environment variable | |
| if os.Getenv("FGA_DEBUG") == "true" { | |
| return true | |
| } | |
| return false | |
| } |
🧰 Tools
🪛 GitHub Check: Lints
[failure] 69-69:
missing whitespace above this line (invalid statement above if) (wsl_v5)
🤖 Prompt for AI Agents
In cmd/root.go around lines 55 to 80, update isDebugMode to correctly detect the
--debug flag without an explicit value, fix the misleading comment and add the
missing blank line for linting: treat any arg equal to "--debug" as true, handle
forms "--debug=true" and "--debug=false" by parsing the value after the '='
(accepting standard boolean strings), keep command-line flag precedence over
FGA_DEBUG (so only check the env var if no flag was present), change the comment
that currently says "Check environment variable first" to accurately reflect
that the code checks the command-line flag first then the env var, and add the
required blank line before the environment-variable check for style compliance.
Description
Note: these will only warn and the actual command will continue
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
closes #608
Review Checklist
mainSummary by CodeRabbit