search

docker / cli

The Docker CLI
Apache License 2.0
4.88k stars 1.92k forks source link

plugins/hooks: Don't show empty hook messages #5078

Closed vvoland closed 4 months ago

vvoland commented 4 months ago

- What I did 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).

- How to verify it Unit test

- Description for the changelog

Don't show empty hints when plugins returns an empty hook message.

- A picture of a cute animal (not mandatory but encouraged)

krissetto commented 4 months ago

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

vvoland commented 4 months ago

Added a debug log (visible only when --debug flag is present).

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 61.34%. Comparing base (4445e77) to head (296a6f5). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ 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 ```