-
Notifications
You must be signed in to change notification settings - Fork 1.8k
builder: avoid duplicate CLI error logging in generated main #14317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
builder: avoid duplicate CLI error logging in generated main #14317
Conversation
|
|
axw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I suggest adding a comment to make it clear that we're intentionally not logging the error though.
|
@Syedowais312 can you please add a changelog entry? It's a small change, but still worthy of an enhancement changelog IMO. |
|
@axw I’ve added a changelog entry and a comment in |
CHANGELOG.md
Outdated
| - `cmd/mdatagen`: Add lint/ordering validation for metadata.yaml (#13781) | ||
| - `pdata/xpdata`: Refactor JSON marshaling and unmarshaling to use `pcommon.Value` instead of `AnyValue`. (#13837) | ||
| - `pkg/exporterhelper`: Expose `MergeCtx` in exporterhelper's queue batch settings` (#13742) | ||
| - `cmd/builder`: Avoid duplicate CLI error logging in generated collector binaries by relying on cobra's error handling. (#14317) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Syedowais312 we shouldn't update this file directly, please see https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#adding-a-changelog-entry
18b3e17 to
70644ec
Compare
|
@axw Thanks for the pointer ,fixed the chloggen entry by removing empty optional fields, validated with make |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14317 +/- ##
=======================================
Coverage 92.10% 92.10%
=======================================
Files 668 668
Lines 41379 41379
=======================================
Hits 38114 38114
Misses 2227 2227
Partials 1038 1038 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @Syedowais312. CI is failing because the generated code in otelcorecol will need changing.
Problem
When running an invalid CLI command (for example
otelcorecol foo), Cobra already prints a user-facing error message.However, the builder-generated
main.goalso logs the same error usinglog.Fatalf, which results in duplicate error output.Solution
This change updates the builder template to:
log.Fatalffrommain()os.Exit(1)Result
Before
After
Notes
cmd/builder/internal/builder/templates/main.go.tmplCloses: #14302