-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
plugins/hooks: Don't show empty hook messages #5078
Conversation
}, | ||
{ | ||
processed: []string{"Some hint", "", "Some other hint"}, | ||
expectedOut: []string{"Some hint", "", "Some other hint"}, |
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.
Are we sure we want to include empty lines if the plugin returns them?
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.
Not sure either, but this was the behavior we had so didn't want to change anything other than just handling the empty message.
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.
fair enough for now
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.
Tbh, I would be okay with removing empty lines (actually that was the first thing I did, but then decided not to change the behavior). I can do that in this PR if we want to, @laurazard any opinions?
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.
ok as a quick fix, we should think of what to do with empty messages when not all are empty (we could probably remove them, at least in the case of the first message being empty)
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 was wondering whether we should do this since, ultimately, plugins should not print valid HookMessage
structs with empty messages if a message is not supposed to be printed, but I don't think we lose anything by being defensive and skipping these on the CLI side.
(I wonder if we should have a debug-level log in the case we skip any for this case, could be better for debugging vs. silently failing)
+ 1 for staying safe. The repercussions of plugins not doing things "correctly" fall on the CLI experience, so we should safeguard it where possible IMHO |
Don't show `Next steps:` with no messages at all when plugin returns an unitialized value of `HookMessage` (zero-initialization sets its type to NextSteps and empty template). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Added a debug log (visible only when |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5078 +/- ##
=======================================
Coverage 61.33% 61.34%
=======================================
Files 298 298
Lines 20691 20703 +12
=======================================
+ Hits 12691 12700 +9
- Misses 7099 7102 +3
Partials 901 901 |
- What I did
Don't show
Next steps:
with no messages at all when plugin returns an unitialized value ofHookMessage
(zero-initialization sets its type to NextSteps and empty template).- How to verify it
Unit test
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)