-
Notifications
You must be signed in to change notification settings - Fork 117
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
fix(commands): Require an argument name before the list of tracing filters when used without a subcommand #7056
Conversation
This looks like a breaking change, does it need to wait for the 1.1.0 release? (I don't mind either way, just checking as the person tagging the release.) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7056 +/- ##
==========================================
+ Coverage 77.24% 77.41% +0.16%
==========================================
Files 310 310
Lines 41833 41830 -3
==========================================
+ Hits 32315 32382 +67
+ Misses 9518 9448 -70 |
I'd be okay with a patch update since it's a very small breaking change. |
Seems ok, just as long as it has something in the changelog. |
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.
Looks good, some optional suggestions about the changelog and testing.
Co-authored-by: teor <[email protected]>
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.
Thanks for this fix!
Motivation
Supplying an incorrect subcommand name should not run start.
This PR does not affect how tracing filters are parsed where the user provided the start subcommand.
Closes #7052
Solution
long
tofilters
field onEntryPoint
(this field is ignored by the tracing component)Related: I tried
EnvFilter::try_new().expect()
, but the format was too permissive, (this was noted in the issue as well.)Review
Anyone can review.
Reviewer Checklist