Feat: Log messages to help with invalid startup options - #699
Conversation
Signed-off-by: Ed Snible <snible@us.ibm.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughPlugin builders now warn when no plugins are registered, while CLI commands provide more specific YAML configuration and load-failure messages. ChangesPlugin build diagnostics
CLI configuration diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![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
🤖 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 `@authbridge/cmd/authbridge-cpex/main.go`:
- Line 104: Standardize startup diagnostics in main across all binaries: at
authbridge/cmd/authbridge-cpex/main.go:104,
authbridge/cmd/authbridge-envoy/main.go:102, and
authbridge/cmd/authbridge-proxy/main.go:150, change the required-config message
to use “must point to a YAML file”; at
authbridge/cmd/authbridge-cpex/main.go:109,
authbridge/cmd/authbridge-envoy/main.go:124, and
authbridge/cmd/authbridge-proxy/main.go:165, use the load-failure wording
“failed to load config %q: %v”.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d84f393-666e-4142-abf5-174fc850479a
📒 Files selected for processing (4)
authbridge/authlib/plugins/registry.goauthbridge/cmd/authbridge-cpex/main.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-proxy/main.go
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
This PR improves diagnostics for invalid startup options — clearer --config errors across the three binaries plus a hint when no plugins are registered (a common "forgot the build tags" footgun). Logic is sound and CI is green. The only feedback is on the wording of the new messages themselves: since clearer messaging is the whole point of the PR, a couple of the strings are worth tightening. All comments are non-blocking.
Areas reviewed: Go (authbridge). Commits: 1, signed-off. CI: passing.
|
|
||
| if *configPath == "" { | ||
| log.Fatal("--config is required") | ||
| log.Fatal("--config is required and must be to a YAML file") |
There was a problem hiding this comment.
suggestion: "--config is required and must be to a YAML file" — "must be to a YAML file" isn't quite grammatical. Suggest "--config is required and must point to a YAML file". Same string appears in authbridge-envoy/main.go:102 and authbridge-proxy/main.go:150. (CodeRabbit flagged this one too.)
| bootCfg, err := config.Load(*configPath) | ||
| if err != nil { | ||
| log.Fatalf("initial config load: %v", err) | ||
| log.Fatalf("initial config load failed to load %q: %v", *configPath, err) |
There was a problem hiding this comment.
nit: "initial config load failed to load %q: %v" reads a bit redundantly ("load … load"). Consider "failed to load config %q: %v". Same wording in authbridge-envoy/main.go:124 and authbridge-proxy/main.go:165.
| return nil, fmt.Errorf("unknown plugin %q (registered: %v)", e.Name, RegisteredPlugins()) | ||
| pluginNames := RegisteredPlugins() | ||
| if len(pluginNames) == 0 { | ||
| slog.Warn("No registered plugins -- Built with --tags or use `go run .` to enable") |
There was a problem hiding this comment.
nit: "No registered plugins -- Built with --tags..." — "Built" reads as past tense; the imperative "Build with --tags" fits the hint better. Same warning at line 341.
Signed-off-by: Ed Snible <snible@us.ibm.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
No Issue. A small PR that adds logging around invalid start options.
(Optional) Testing Instructions
To start locally,
Summary by CodeRabbit
Bug Fixes
--config, including clearer guidance that it must be a YAML file.Diagnostics