OnlyValidArgs does not take into account the new ValidArgsFunction

[davidovich]

When you use the new recommended ValidArgsFunction, you loose the possibility to use the OnlyValidArgs validation function. As the documentation says that ValidArgs and ValidArgsFunction are mutually exclusive, programs that use the OnlyValidArgs validator break when transitioning to ValidArgsFunction.

As both config options are mutually exclusive, the OnlyValidArgs function should use the the result of ValidArgsFunction if it is available.

I believe this is a regression that appeared when implementing the ValidArgsFunction in #1035, which states that it is a dynamic version of ValidArgs, hence should support the validator also.

Note that #841 moves the OnlyValidArgs to a private function, and will suffer from the same problem.

[marckhouzam]

Thanks @davidovich for bringing this up. Of course you make a valid point. I hesitated about this question when implementing #1035.

OnlyValidArgs() currently allows to check a list of valid arguments (ValidArgs) before invoking the command itself. This seems like an low-cost and efficient validation step.

On the other hand including a verification of ValidArgsFunction could potentially trigger expensive computation. For example let's take:

kubectl describe pod mypod-12345

OnlyValidArgs() would call ValidArgsFunction which would contact the kubernetes cluster to fetch the list of pods to make sure the requested one is valid. Only once validated would it perform the real kubectl command. This is an extra operation that would noticeably slow down the user-requested operation.

And say the argument is not valid, Cobra will print a generic "invalid argument" message. By instead letting kubectl perform the command with the invalid argument will allow kubectl to print a more specific error message that will be clearer to the user.

With that in mind, I am of the opinion we should not include ValidArgsFunction in the OnlyValidArgs() logic.

But we should probably document this behaviour better.

What do you think?

[davidovich]

@marckhouzam Yes, I believe this could be prohibitive to always call the function, but in some local scenarios, I could also be fine performance wise. A configuration option could control if the OnlyValidArgs() calls the ValidArgsFunction, as the implementer has better knowledge of the performance characteristics.

But I also agree that cobra can only do so much generically, so aside from that, a better documentation regarding this fact would be a gain IMHO.

[umarcor]

Note that #841 moves the OnlyValidArgs to a private function, and will suffer from the same problem.

This is not correct. In #841, OnlyValidArgs is deprecated because it is redundant. Precisely, ValidArgs is made orthogonal to the options related to the number of arguments. See the documentation in #841:

Field ValidArgs of type []string can be defined in Command, in order to report an error if there are any positional args that are not in the list. This validation is executed implicitly before the validator defined in Args.

NOTE: OnlyValidArgs and ExactValidArgs(int) are now deprecated. ArbitraryArgs and ExactArgs(int) provide the same functionality now.

Therefore, with #841, you should be able to define ValidArgs together with ArbitraryArgs for having both conditions. I'm not sure about ValidArgsFunction, because that was implemented independently without explicit feedback. Anyway, please let me know if you find any issue when using #841.

[github-actions]

This issue is being marked as stale due to a long period of inactivity

[johnSchnake]

Based on the last comment; its just waiting for the #841 to merge. Leaving open until then.

[umarcor]

@johnSchnake my understanding is that this issue is unrelated to #841 indeed. #841 was created before ValidArgsFunction existed in the codebase.

In #841, OnlyValidArgs is deprecated (not removed) because it's a redundant function (equivalent to ValidArgs + ArbitraryArgs). The behaviour with regard to ValidArgsFunction is unmodified, which is what this is issue about.