-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[service]: add new subcommand to examine the initial configuration #11775
base: main
Are you sure you want to change the base?
[service]: add new subcommand to examine the initial configuration #11775
Conversation
Co-authored-by: Mauri de Souza Meneguzzo <[email protected]>
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (90.32%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11775 +/- ##
==========================================
- Coverage 91.38% 91.38% -0.01%
==========================================
Files 468 469 +1
Lines 25610 25641 +31
==========================================
+ Hits 23404 23432 +28
- Misses 1790 1792 +2
- Partials 416 417 +1 ☔ View full report in Codecov by Sentry. |
otelcol/command_examine.go
Outdated
if err != nil { | ||
return fmt.Errorf("error while marshaling to yaml: %w", err) | ||
} | ||
log.Printf("\n%s", b) |
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.
Can you change to fmt.Printf here? This output is likely to be piped/redirected and I would rather not have a random timestamp at the start of the output.
Reviewed again, LGTM. Thanks for adding a test to check the tool output. |
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
@mx-psi I've addressed the review comments and added a feature flag for this new command. Please take a look! |
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
@mx-psi Thank you for your review. Once the CI is green, this should be good to go. |
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.
One thing left to address (not necessarily on this PR) is sensitive values. This would leak any sensitive values, so we need to make sure the user understands this
0e35989
to
954fe99
Compare
Description
Why is this useful?
--config
sources.Usage
Link to tracking issue
Fixes ##11479
Testing
Added unit test cases.
Documentation
Updated readme.