fix(cli): accept --config and --log-level before the subcommand - #776
Conversation
Closes #772. Both flags are pre-parsed out of argv before the logger exists, so they behave globally — but the top-level parser declared neither, and writing them where a user naturally would was rejected: $ ofelia --config /etc/ofelia/config.ini validate unknown flag `config' $ ofelia validate --config /etc/ofelia/config.ini (works) Nothing about `--config` suggests it only exists after the subcommand, and it is the flag that decides which file every subcommand reads. Two changes, and the second is the one that matters. Declaring the pair on the parser stops the rejection, but the path still did not arrive: daemon, validate and config-show each carried their own `--config` with `default:"/etc/ofelia/config.ini"`, and go-flags applies that default during parsing, overwriting the pre-parsed value the command was built with. `ofelia --config=x validate` then read /etc/ofelia/config.ini while reporting success on a file it had never opened. The default now lives once, on the global option. Without the tag on the subcommands, go-flags leaves the field alone when the flag is absent, so the value from the pre-parse survives — and when the flag is given after the subcommand the pre-parser has already seen it too, so both positions agree. The e2e tests cover both halves: that neither position is rejected, and that the requested path actually reaches the command. The second would have failed on the first change alone. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
✅ Mutation Testing ResultsMutation Score: 66.67% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
==========================================
- Coverage 89.31% 89.27% -0.04%
==========================================
Files 88 88
Lines 12053 12075 +22
==========================================
+ Hits 10765 10780 +15
- Misses 994 998 +4
- Partials 294 297 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
This PR fixes CLI ergonomics in Ofelia’s top-level command parser so --config and --log-level behave like true global flags (usable before or after the subcommand) and ensures the chosen --config path actually reaches the command implementation rather than being overwritten by per-subcommand defaults.
Changes:
- Introduce a shared
globalOptionsstruct and register it on the top-level go-flags parser soofelia --config=… <cmd>andofelia <cmd> --config=…both parse. - Remove per-subcommand
--configdefault tags to prevent go-flags defaults from overwriting the pre-parsed config path. - Add e2e coverage to assert both “parses in either position” and “the requested config path is honored.”
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ofelia.go |
Adds global option declarations and registers them on the top-level parser. |
e2e/cli_exit_codes_test.go |
Adds e2e tests for global flag positioning and honoring the config path. |
cli/validate.go |
Removes --config default so pre-parsed/global config selection isn’t overwritten. |
cli/daemon.go |
Removes --config default so global config selection isn’t overwritten. |
cli/config_show.go |
Removes --config default so global config selection isn’t overwritten. |
Review finding from Copilot on PR #776. Declaring the flag globally fixed the rejection for daemon, validate and config-show, but doctor was still constructed without the pre-parsed value, so `ofelia --config=x doctor` was accepted and then disregarded: it reported on /etc/ofelia/config.ini and told the user the file was missing. Accepting a flag and ignoring it is worse than rejecting it — nothing says the path was dropped. doctor is also the reason the default cannot simply be handed to every command: given no path it searches well-known locations, and a pre-filled default would take that away silently. The default is therefore resolved in run() for the commands that need a concrete file, while doctor keeps the raw value, so an absent flag still means 'go and find it'. Verified against the binary in all four combinations: doctor with the flag before and after the subcommand uses the given file, doctor without it still auto-detects (./ofelia.ini), and validate is unchanged. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Closes #772.
The flags looked global and were not
--log-leveland--configare pre-parsed out of argv before the logger exists, so they behave globally — but the top-level parser declared neither:Nothing about
--configsuggests it only exists after the subcommand, and it is the flag that decides which file every subcommand reads.The half that a "does it parse" fix would have missed
Declaring the pair on the parser stops the rejection — but the path still did not arrive.
daemon,validateandconfig-showeach carried their own--configwithdefault:"/etc/ofelia/config.ini", and go-flags applies that default during parsing, overwriting the pre-parsed value the command was constructed with.So after the obvious fix, this happened:
The flag was accepted and then ignored. The default now lives once, on the global option; without the tag on the subcommands go-flags leaves the field alone when the flag is absent, so the pre-parsed value survives. When the flag is given after the subcommand the pre-parser has already seen it too, so both positions agree.
Verified against the binary
--config=<good> validateunknown flagvalidate --config=<good>--log-level debug versionunknown flag--config=<missing> validateunknown flagvalidate --config=<missing>validatewith no flag/etc/ofelia/config.iniTests
Two e2e tests, because the two halves fail differently: one that neither position is rejected, and one that the requested path actually reaches the command. The second would still have failed after the parser-only change — it asserts the error names the config that was asked for, not the compiled-in default.
Test plan
go test ./...— greengo test -race -tags=e2e ./e2e/...— greengolangci-lint runincl.--build-tags="e2e unix"— 0 issueslefthook run pre-push— exit 0