Skip to content

cli-plugins: separate hook types from manager and refactor#6859

Open
thaJeztah wants to merge 15 commits intodocker:masterfrom
thaJeztah:plugin_metadata
Open

cli-plugins: separate hook types from manager and refactor#6859
thaJeztah wants to merge 15 commits intodocker:masterfrom
thaJeztah:plugin_metadata

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Mar 13, 2026

cli-plugins/manager: Plugin.RunHook: improve error message

Currently, the error was a plain "exit status 1"; make the error
message more informative if we need it :)

cli-plugins/manager: simplify ctx-cancel check

cli-plugins/manager: refactor for easier debugging

Extract the code inside the loop to a closure, so that we can more
easily set up debug-logging.

cli-plugins/manager: move HookPluginData to hooks.Request

Separate types used by plugins from the manager code.

cli-plugins/hooks: rename HookMessage to Response

cli-plugins/hooks: rename HookType to ResponseType

Rename the type to match the struct it's used for. Also;

  • Fix the type of the NextSteps const
  • Don't use iota for values; the ResponseType is used as
    part of the "wire" format, which means that plugins using
    the value can use a different version of the module code;
    using iota increases the risk of (accidentally) changing
    values, which would break the wire format.

cli-plugins/hooks: add JSON labels, omitzero

Add labels to define the expected casing and don't serialize
empty fields.

cli-plugins/hooks: move template utils separate from render code

These utilities are used by CLI-plugins; separate them from the render
code, which is used by teh CLI-plugin manager.

cli-plugins/hooks: update tests

  • add basic unit-test for the template utilities
  • make sure the template parsing tests test both the current
    template produced by the utilities, as well as a fixture
  • rewrite the printer test to use fixtures
  • use blackbox testing ("hooks_test")

cli-plugins/hooks: slight tweaks in templates

  • use %q instead of manually quoting the string
  • use %d instead of manually converting the number to a string

cli-plugins/hooks: detect if templating is needed

cli-plugins/hooks: limit maximum number of lines / messages

cli-plugins/hooks: update godoc

cli-plugins/hooks: add commandInfo type for templating

Define a local type for methods to expose to the template, instead of
passing the cobra.Cmd. This avoids templates depending on features
exposed by Cobra that are not part of the contract, and slightly
decouples the templat from the Cobra implementation.

cli-plugins/hooks: simplify templating formats

This allows for slighly cleaner / more natural placeholders, as it
doesn't require the context (.) to be specified;

  • {{command}} instead of {{.Command}} or {{command .}}
  • {{flagValue "my-flag"}} instead of {{.FlagValue "my-flag"}} or {{flagValue . "my-flag"}}`

cli-plugins/hooks: PrintNextSteps: slight cleanup

  • skip aec to construct the formatting and use a const instead
  • skip fmt.Println and write directly to the writer

- Human readable description for the release notes

Go SDK: cli-plugins/manager: deprecate `HookPluginData` and move to `cli-plugins/hooks.Request`
Go SDK: cli-plugins/hooks: deprecate `HookMessage ` and rebane to `cli-plugins/hooks.Response`
Go SDK: cli-plugins/hooks: deprecate `HookType` and rebane to `cli-plugins/hooks.ResponseType `

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2026

Codecov Report

❌ Patch coverage is 59.30233% with 35 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cli-plugins/hooks/template.go 64.10% 7 Missing and 7 partials ⚠️
cli-plugins/manager/hooks.go 51.72% 14 Missing ⚠️
cli-plugins/manager/plugin.go 0.00% 7 Missing ⚠️

📢 Thoughts on this report? Let us know!

Currently, the error was a plain "exit status 1"; make the error
message more informative if we need it :)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Extract the code inside the loop to a closure, so that we can more
easily set up debug-logging.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Separate types used by plugins from the manager code.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Rename the type to match the struct it's used for. Also;

- Fix the type of the NextSteps const
- Don't use iota for values; the ResponseType is used as
  part of the "wire" format, which means that plugins using
  the value can use a different version of the module code;
  using iota increases the risk of (accidentally) changing
  values, which would break the wire format.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the plugin_metadata branch 2 times, most recently from 9b06ef5 to a2448d5 Compare March 18, 2026 13:50
@thaJeztah thaJeztah changed the title WIP: separate hook types from manager cli-plugins: separate hook types from manager and refactor Mar 18, 2026
@thaJeztah thaJeztah added impact/changelog impact/deprecation status/2-code-review area/plugins kind/refactor PR's that refactor, or clean-up code area/go-sdk Changes affecting the Go SDK impact/go-sdk Noteworthy (compatibility changes) in the Go SDK labels Mar 18, 2026
@thaJeztah thaJeztah added this to the 29.3.1 milestone Mar 18, 2026
@thaJeztah thaJeztah marked this pull request as ready for review March 18, 2026 14:12
@thaJeztah thaJeztah force-pushed the plugin_metadata branch 7 times, most recently from f64350a to d8c7ee1 Compare March 19, 2026 09:42
vvoland
vvoland previously approved these changes Mar 19, 2026
// representation of this type when their hook subcommand
// is invoked.
type Response struct {
Type ResponseType `json:"Type,omitzero"`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I was considering if we should perhaps drop the omitzero to make it usable as a discriminator (and more clearly have different responses in future if we need)

We should have a Type in Request as well, but that's for a follow-up.

Let me push that change, then we should be good to go.

Add labels to define the expected casing and don't serialize
empty fields.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These utilities are used by CLI-plugins; separate them from the render
code, which is used by teh CLI-plugin manager.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- add basic unit-test for the template utilities
- make sure the template parsing tests test both the current
  template produced by the utilities, as well as a fixture
- rewrite the printer test to use fixtures
- use blackbox testing ("hooks_test")

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use `%q` instead of manually quoting the string
- use `%d` instead of manually converting the number to a string

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Define a local type for methods to expose to the template, instead of
passing the cobra.Cmd. This avoids templates depending on features
exposed by Cobra that are not part of the contract, and slightly
decouples the templat from the Cobra implementation.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This allows for slighly cleaner / more natural placeholders, as it
doesn't require the context (`.`) to be specified;

- `{{command}}` instead of `{{.Command}}` or `{{command .}}`
- `{{flagValue "my-flag"}}` instead of `{{.FlagValue "my-flag"}} or `{{flagValue . "my-flag"}}`

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- skip aec to construct the formatting and use a const instead
- skip fmt.Println and write directly to the writer
- move newlines outside of the "bold" formatting

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/go-sdk Changes affecting the Go SDK area/plugins impact/changelog impact/deprecation impact/go-sdk Noteworthy (compatibility changes) in the Go SDK kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants