Conversation
pkg/cmdutil/json_flags.go
Outdated
| return JSONFlagError{fmt.Errorf("Specify one or more comma-separated fields for `--json`:\n %s", strings.Join(fields, "\n "))} | ||
| } | ||
| return c.Parent().FlagErrorFunc()(c, e) | ||
| return cmd.Parent().FlagErrorFunc()(c, e) |
There was a problem hiding this comment.
This might be a workaround, but there is still a problem of calling the parent FlagErrorFunc but passing it the wrong *cobra.Command reference as argument.
The core of the problem is that FlagErrorFunc is inherited by subcommands, but that up to now we've never used AddJSONFlags on a command that has subcommands. This FlagErrorFunc here is only supposed to be applied on the command with the --json flag, but accidentally applies to subcommands. Furthermore, when this custom FlagErrorFunc tried to invoke the parent FlagErrorFunc, it passed in the wrong argument to it.
I think a proper fix would be twofold:
- Only ever raise the JSONFlagError if
c == cmd(in addition to the already existing condition); - When calling
FlagErrorFunconc.Parent(), you should pass the same parent as argument to the func.
Finally, for good measure, you could also check if c.Parent() even exists (root commands will not have a parent) and only call its FlagErrorFunc if it does. We don't yet use AddJSONFlags on any root command, but in case we do, it would be nice if it doesn't panic.
Does all this make sense?
There was a problem hiding this comment.
I initially had the passed argument set to c.Parent() but then it seemed to me that this is rather meant to be the command on which the error occured.
This might also make more sense for 1. where the json error should only be raised on the command where the flag was registered as it isn't persistent and this way the fields are also correct.
Parent check makes sense (and is also done in the default func).
So i would agree on both the checks, but i am not so sure about the passed argument.
There was a problem hiding this comment.
but then it seemed to me that this is rather meant to be the command on which the error occured.
Ah! You might be right. Although, then I really don't know how Cobra expects us to override the FlagErrorFunc down the line while still respecting potentially defined FlagErrorFunc handlers of parent commands 🤔 But since we don't use SetFlagErrorFunc heavily across the codebase, I think your approach is actually fine and makes more sense than what I suggested.
67065ef to
7c443ab
Compare
fix #4612