Issue warning on unsupported CDI hook #1000
Conversation
To allow for CDI hooks to be added gradually we provide a generic no-op hook for unrecognised subcommands. This will log a warning instead of erroring out. Signed-off-by: Evan Lezar <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a generic no-op hook that logs a warning for unsupported subcommands, allowing CDI hooks to be added gradually without causing errors.
- Added WarnOnUnsupportedSubcommand functions in both CDI hook and CTK hook implementations
- Updated the main action for CDI hook and the hook command action for CTK to use the new warning function
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/nvidia-cdi-hook/commands/commands.go | Added a new helper function to log a warning for unsupported subcommands |
| cmd/nvidia-ctk/hook/hook.go | Updated the hook command to call the warning function when an unsupported subcommand is invoked |
| cmd/nvidia-cdi-hook/main.go | Configured the CDI hook main command to warn when no valid subcommand is provided |
Comments suppressed due to low confidence (2)
cmd/nvidia-ctk/hook/hook.go:44
- [nitpick] The inline Action definition is clear now; if additional similar actions are added in other hooks, consider centralizing this behavior to reduce code duplication.
Action: func(ctx *cli.Context) error {
cmd/nvidia-cdi-hook/main.go:55
- [nitpick] The Action hook configuration aligns with the generic no-op behavior; ensure that any future extensions continue to follow this consistent pattern.
c.Action = func(ctx *cli.Context) error {
| @@ -36,3 +36,8 @@ func New(logger logger.Interface) []*cli.Command { | |||
| cudacompat.NewCommand(logger), | |||
| } | |||
| } | |||
|
|
|||
| func WarnOnUnsupportedSubcommand(logger logger.Interface, c *cli.Context) error { | |||
| logger.Warningf("Unsupported hook or arguments %v", c.Args()) | |||
There was a problem hiding this comment.
The warning message is generic; consider including additional context about the subcommand to aid in debugging if necessary.
| logger.Warningf("Unsupported hook or arguments %v", c.Args()) | |
| logger.Warningf("Unsupported subcommand '%s' or arguments %v", c.Command.Name, c.Args()) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -41,6 +41,9 @@ func (m hookCommand) build() *cli.Command { | |||
| hook := cli.Command{ | |||
| Name: "hook", | |||
There was a problem hiding this comment.
From the CLI libraries perspective -- does this define a subcommand named "hook"?
There was a problem hiding this comment.
if so, the code comment maybe should be "create 'hook' subcommand" (just noting this as I try to understand the code here)
| @@ -41,6 +41,9 @@ func (m hookCommand) build() *cli.Command { | |||
| hook := cli.Command{ | |||
| Name: "hook", | |||
| Usage: "A collection of hooks that may be injected into an OCI spec", | |||
| Action: func(ctx *cli.Context) error { | |||
| return commands.WarnOnUnsupportedSubcommand(m.logger, ctx) | |||
| }, | |||
| } | |||
|
|
|||
| hook.Subcommands = commands.New(m.logger) | |||
There was a problem hiding this comment.
oh. Hm maybe "hook" is the command, and sub-commands are one level deeper.
| @@ -51,6 +51,11 @@ func main() { | |||
| c.Usage = "Command to structure files for usage inside a container, called as hooks from a container runtime, defined in a CDI yaml file" | |||
| c.Version = info.GetVersionString() | |||
|
|
|||
| // We add an action to the hook | |||
There was a problem hiding this comment.
The comment "We add an action to the hook" does not help me understand the code better :).
I think maybe instead of "We add an action to the hook" we can write "Add a default action to this CLI. It is executed when the user-provided sub-command is unknown"
I still wonder about the hierarchy provided by this CLI library. There is a CLI. The CLI has various commands. Each command has subcommands. Is that true? Or is the CLI the command?
I ask because I wonder: what is a hook? A command? A subcommand?
Again, this is about code readability. I have built CLIs with various different libraries and ecosystems and I know everyone calls things differently... :)
There was a problem hiding this comment.
Now that I see
./nvidia-cdi-hook foo
and
./nvidia-ctk hook foo
I realize that we might have both: hook being the CLI, and hook also being a command in a CLI.. ouch. OK.
| @@ -51,6 +51,11 @@ func main() { | |||
| c.Usage = "Command to structure files for usage inside a container, called as hooks from a container runtime, defined in a CDI yaml file" | |||
| c.Version = info.GetVersionString() | |||
|
|
|||
| // We add an action to the hook | |||
| c.Action = func(ctx *cli.Context) error { | |||
| return commands.WarnOnUnsupportedSubcommand(logger, ctx) | |||
There was a problem hiding this comment.
I suppose a decisive design decision here is "emit warning on stderr, and exit 0".
Is that right? I think we should maybe document that via code comment.
There was a problem hiding this comment.
Thank you @elezar! Merge this at will. We talked about offline, and of course it's ready to land.
In terms of code readability I have however left a few questions / remarks / thoughts. Please reply and/or adjust code at your own discretion! :)
|
To allow for CDI hooks to be added gradually we provide a generic no-op hook for unrecognised subcommands. This will log a warning instead of erroring out.
For example: