hooks: include plugin name in hook data #5030
Merged
+137
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krissetto
approved these changes
Apr 19, 2024
vvoland
reviewed
Apr 19, 2024
laurazard
force-pushed
the
hooks-plugin-name
branch
from
71fe473 to
7bcce0b
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5030 +/- ##
==========================================
+ Coverage 61.05% 61.10% +0.05%
==========================================
Files 295 295
Lines 20647 20656 +9
==========================================
+ Hits 12605 12621 +16
+ Misses 7146 7138 -8
- Partials 896 897 +1 |
laurazard
force-pushed
the
hooks-plugin-name
branch
from
7bcce0b to
6a1d249
Compare
|
I yet have to dive into the changes in this PR, but wondering if there's cases where "global" flags could cause issues, e.g. |
|
It operates on the args already processed by cobra, so it also works fine with global flags. |
krissetto
reviewed
Apr 22, 2024
vvoland
reviewed
Apr 22, 2024
laurazard
force-pushed
the
hooks-plugin-name
branch
from
6a1d249 to
975696c
Compare
Before, for plugin commands, only the plugin name (such as `buildx`) would be both included as `RootCmd` when passed to the hook plugin, which isn't enough information for a plugin to decide whether to execute a hook or not since plugins implement multiple varied commands (`buildx build`, `buildx prune`, etc.). This commit changes the hook logic to account for this situation, so that the the entire configured hook is passed, i.e., if a user has a hook configured for `buildx imagetools inspect` and the command `docker buildx imagetools inspect alpine` is called, then the plugin hooks will be passed `buildx imagetools inspect`. This logic works for aliased commands too, so whether `docker build ...` or `docker buildx build` is executed (unless Buildx is disabled) the hook will be invoked with `buildx build`. Signed-off-by: Laura Brehm <laurabrehm@hey.com> hooks: include full match when invoking plugins Signed-off-by: Laura Brehm <laurabrehm@hey.com>
laurazard
force-pushed
the
hooks-plugin-name
branch
from
975696c to
9d8320d
Compare
krissetto
approved these changes
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
Before, for plugin commands, only the plugin name (such as
buildx) would be included inRootCmdwhen passed to the hook plugin, which isn't enough information for a plugin to decide whether to execute a hook or not since plugins implement multiple varied commands (buildx build,buildx prune, etc.).- How I did it
This commit changes the hook logic to account for this situation, so that both the plugin name and base command will get passed (separated by a SPACE, such as
buildx prune).This logic works for aliased commands too, so whether
docker build ...ordocker buildx buildis executed (unless Buildx is disabled) the hook will be invoked withbuildx build.- A picture of a cute animal (not mandatory but encouraged)