Distinguish an explicitly passed flag from its default value - #419
Open
dpage wants to merge 2 commits into
Open
Conversation
Both services documented the precedence of built-in defaults, then the configuration file, then command-line flags, but neither could tell an explicitly passed flag from one left at its registered default: each decided the question by comparing the flag's current value against its own hardcoded default. The collector did this for `-pg-port` and `-pg-sslmode`, whose defaults are real values rather than zero values, and the alerter did the same for its whole `applyFlagOverrides` set, where the zero-value defaults mostly masked the problem. That comparison conflates two genuinely different situations, and it gets both of them wrong. A configuration file value that happens to equal a flag's default was overwritten even though nothing was passed on the command line, and a flag passed explicitly with its default value was ignored: a file setting `datastore.port: 6000` survived `-pg-port 5432`, which is precisely the case an operator would reach for to force the standard port back. The new `pkg/flagutil` helper records the flags a `flag.FlagSet` actually saw, via `flag.Visit`, and both binaries now consult that set instead of comparing values. The collector threads the set through `loadConfiguration` into `ApplyFlags`, whilst the alerter carries it in the `flagOverrides` bundle that both startup and the SIGHUP reload path already share, so overrides survive a reload exactly as before. Flag names became constants in both binaries so registration and lookup cannot drift apart. Closes #389
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds explicit command-line flag tracking. Collector and alerter configuration now preserve file values for omitted flags and apply explicitly supplied flags, including values equal to built-in defaults. ChangesExplicit Flag Precedence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Command-line flags
participant Collector as Collector
participant Config as Configuration loader
participant Alerter as Alerter
CLI->>Collector: Parse flags and call flagutil.Passed
Collector->>Config: Pass flagutil.Set to loadConfiguration
Config->>Config: Apply only explicitly passed flags
CLI->>Alerter: Parse flags and capture flagOverrides
Alerter->>Config: Apply flagOverrides at startup or SIGHUP reload
Config->>Config: Read password file only for non-empty passed paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 44 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@alerter/src/cmd/ai-dba-alerter/main.go`:
- Around line 349-358: Update the configuration handling around
flagDBPasswordFile so cfg.Datastore.PasswordFile is always assigned
o.DBPasswordFile whenever the flag is explicitly passed, including an empty
value, before the conditional file read. Preserve the non-empty path read and
error handling, and add a regression test covering a configured password_file
overridden by an explicit empty flag.
In `@collector/src/main.go`:
- Line 97: Run gofmt across all six unformatted files under pkg/, then fix
QF1001 and ST1005 in pkg/fileutil/fileutil.go:147-148 while preserving behavior
and idiomatic error text. No direct changes are required at
collector/src/main.go:97, collector/src/config.go:101-129,
collector/src/config_test.go:473-736, or collector/src/main_test.go:25-315;
verify the repository-wide Go checks afterward.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7570ec31-0bcf-450f-8523-223b26f64510
📒 Files selected for processing (13)
.claude/golang-expert/testing-strategy.mdalerter/src/cmd/ai-dba-alerter/flagoverrides_test.goalerter/src/cmd/ai-dba-alerter/main.goalerter/src/cmd/ai-dba-alerter/main_test.gocollector/src/config.gocollector/src/config_test.gocollector/src/main.gocollector/src/main_test.godocs/changelog.mddocs/getting-started/configuration/alerter.mddocs/getting-started/configuration/collector.mdpkg/flagutil/flagutil.gopkg/flagutil/flagutil_test.go
… flag An explicitly empty -db-password-file skipped the immediate read but left the path the configuration file named in place, so the later cfg.LoadPassword during startup and on every SIGHUP reload still read that file. The flag now always replaces the configured path, including with an empty value, whilst only a non-empty path is read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The collector and the alerter both document configuration
precedence as built-in defaults, then the configuration file, then
command-line flags, but neither could tell a flag the operator
actually passed from one sitting at its registered default: both
decided the question by comparing the flag's value against its own
hardcoded default. The collector did this for
-pg-portand-pg-sslmode, whose defaults are real values (5432and"prefer"), and the alerter did it throughoutapplyFlagOverrides,where zero-value defaults mostly masked the problem.
The consequence cuts both ways: a configuration file value that
happens to equal a flag's default was overwritten even though
nothing was passed, and a flag passed explicitly with its default
value was ignored, so a file setting
datastore.port: 6000survived-pg-port 5432.This adds
pkg/flagutil, a small shared helper that records theflags a
flag.FlagSetactually saw viaflag.Visit, and switchesboth binaries onto it. The collector threads the set through
loadConfigurationinto(*Config).ApplyFlags; the alerter carriesit in the
flagOverridesbundle that startup and the SIGHUP reloadpath already share, so command-line overrides still survive a
reload. Flag names became constants in both binaries so registration
and lookup cannot drift apart. The documented precedence order is
unchanged; it simply now holds in every case.
One behavioural nicety worth calling out: an explicitly empty
-db-password-filein the alerter is treated as "no password file"rather than as a path to read, which would otherwise fail on the
empty path.
Test plan
pkg/flagutiltests coverPassedwith no flags, with asubset, with a flag carrying its own default value, and with a nil
FlagSet, plusHason a nilSet: 100% coverage of the package.behaviour from the new: a configuration value coinciding with a
flag default survives when the flag is absent, and an explicitly
passed default value still wins. A further test drives the whole
loadConfigurationpath with-pg-port 5432against a filesetting
port: 6000.applyFlagOverrides, includingthe explicitly empty password-file case.
ApplyFlags100%,applyFlagOverrides100%,pkg/flagutil100%, andloadConfigurationlifted from 86.4% to 90.9%.cd collector && make test-allandcd alerter && make test-allboth pass, linting included;
go test ./...passes forpkg.only a flag you actually pass overrides the file, and
docs/changelog.mdrecords the fix.Closes #389
🤖 Generated with Claude Code
https://claude.ai/code/session_01N44XgdgR2msyNydAao5vXY
Summary by CodeRabbit
Bug Fixes
Documentation