Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate ExactValidArgs and test combinations of args validators #1643

Merged
merged 7 commits into from Sep 10, 2022

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Mar 22, 2022

This is a subset of #841, which was approved by @jharshman and reviewed by @marckhouzam .

Fix #838 and Fix #745.

The original validators care about the number of arguments that a command accepts: NoArgs, ArbitraryArgs, MinimumNArgs(int), MaximumNArgs(int), ExactArgs(int), RangeArgs(min, max). We can consider it an enumeration of 6 items.

When ValidArgs was introduced, up to six additional validators might have been added, in order to provide functions covering the full combination matrix. Certainly, ValidArgs is a boolean condition which is orthogonal to Args, because it is focused on the content of the args, not the number of them. Therefore, the full matrix covers 6 x 2 = 12 combinations. However, NoArgs + ValidArgs == NoArgs + !ValidArgs, so 11.

Nonetheless, the implementation was partial and a single one was introduced. Later, a second one was added. Still, there are three combinations missing, which is what was pointed out in #838 at first.

Hence, this PR enforces the orthogonality in the code by moving the validation of ValidArgs from args.go to command.go, so that:

  • Any validator can be used along with ValidArgs. ValidArgs is checked first, and then the defined validator.
  • OnlyValidArgs and ExactValidArgs are deprecated, but not removed.

The README is updated accordingly.

@marckhouzam
Copy link
Collaborator

Thanks for splitting this out @umarcor. I'll have a look soon.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

I hadn't realized before, but I believe this is a breaking change. I don't know if it was discussed before, so apologies if I missed something.

But note that I believe it would have broken kubectl had kubectl not recently stopped using ValidArgs in favor of ValidArgsFunction().
Let me try to explain using kubectl.

Let's take the kubectl expose command which works on a subset of kubernetes resource types.
For simplicity let's say it only works on deployment and pod (it actually applies to 3 other types of resources, but it does not matter for this example).
So, for the first argument, the user must specify one of the two valid resource types.

Originally, ValidArgs was only used for shell completion, as can be see in the following comment:

cobra/command.go

Lines 66 to 67 in 5414d3d

// ValidArgs is list of all valid non-flag arguments that are accepted in shell completions
ValidArgs []string
(which should technically be changed by this PR).

So, to help the user, ValidArgs was set to {"deployment", "pod"} as the two possible completions.
So, when using shell completion the user would end up with either kubectl expose deployment or kubectl expose pod.

Now, and here is the problem, kubectl accepts multiple "aliases" for its resource types.
So, instead of using deployment I can just as well use: deploy, deployments, deploy.app, deploy.apps, deployment.app, deployments.apps, etc.
This is not a critical point for shell completion which just guides the user, but it is an important feature for the proper functioning of kubectl itself.

If we only allow specified ValidArgs as arguments, as suggested by this PR, such a scenario would stop working.

I believe kubectl is safe now that it uses ValidArgsFunction(), but it shows that we could affect other users of Cobra with this change.

I think we would need to make this opt-in if we are to move forward with this change.
But that is pretty much what we have now, where a program can choose to enforce ValidArgs or not, by specifying what validators to use.

user_guide.md Outdated
An example of setting the custom validator:
If `Args` is undefined or `nil`, it defaults to `ArbitraryArgs`.

Field `ValidArgs` of type `[]string` can be defined in `Command`, in order to report an error if there are any
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:
"The ValidArgs field of any Command can optionally be set to a list of valid values for positional args. If set, an error will be reported if any args are not in the specified list. This validation is executed ..."

@umarcor
Copy link
Contributor Author

umarcor commented Mar 27, 2022

@marckhouzam thanks for a very interesting analysis! It seems that the "main" purpose of ValidArgs changed since it was introduced in 2015:

// List of all valid non-flag arguments, used for bash completions *TODO* actually validate these
ValidArgs []string
`Legacy` (or default) the rules are as follows:
- root commands with no subcommands can take arbitrary arguments
- root commands with subcommands will do subcommand validity checking
- subcommands will always accept arbitrary arguments and do no subsubcommand validity checking

`None` the command will be rejected if there are any left over arguments after parsing flags.

`Arbitrary` any additional values left after parsing flags will be passed in to your `Run` function.

`ValidOnly` you must define all valid (non-subcommand) arguments to your command. These are defined in a slice name ValidArgs. For example a command which only takes the argument "one" or "two" would be defined as:
Validation of positional arguments can be specified using the `Args` field.

The follow validators are built in:

- `NoArgs` - the command will report an error if there are any positional args.
- `ArbitraryArgs` - the command will accept any args.
- `OnlyValidArgs` - the command will report an error if there are any positional args that are not in the ValidArgs list.
- `MinimumNArgs(int)` - the command will report an error if there are not at least N positional args.
- `MaximumNArgs(int)` - the command will report an error if there are more than N positional args.
- `ExactArgs(int)` - the command will report an error if there are not exactly N positional args.
- `RangeArgs(min, max)` - the command will report an error if the number of args is not between the minimum and maximum number of expected args.

At that point, any of MinimumNArgs, MaximumNArgs, ExactArgs or RangeArgs (which are mutually exclusive) could be combined with OnlyValidArgs, by writing a custom validation function.
However, the tests did not include those use cases.

func ExactValidArgs(n int) PositionalArgs {
	return func(cmd *Command, args []string) error {
		if err := ExactArgs(n)(cmd, args); err != nil {
			return err
		}
		return OnlyValidArgs(cmd, args)
	}
}

So, the inconsistency was introduced in #765.
MinimumNValidArgs, MaximumNValidArgs and RangeValidArgs were missing.

It was not obvious back then, that the implementation should have been a generic helper to combine validators.
MatchAll was proposed some months later (#896), which provided the generic solution.
ExactValidArgs could have been explicitly deprecated in #896 in favour of showcasing a generic solution, but it was overlooked (even though we did discuss about it).

Note that this PR was created in March 2019 (#841), between #765 (October 2018) and #896 (June 2019).

Note also that there was no reference to the completion feature in neither of #284, #765, #841 or #896, despite ValidArgs apparently still being used for that purpose as well.
It seems that it was mostly forgotten until you started to work on it 2 years ago.

Overall, I think that enforcing the valid args in the kubectl example you explained was not a supported use case until you reworked and extended the usability of the completion feature.
Nevertheless, I agree there is no need to apply the OnlyValidArgs validator implicitly, because the work introduced in the codebase since this PR was proposed makes it sensible to instead document how to do it explicitly.
Therefore, I will update this PR to:

As a result, the outcomes of this PR will be:

  • Deprecate but not remove ExactValidArgs.
  • Add tests to check the combination of built-in validators which are orthogonal.
  • Document how to combine the built-in validators through the built-in combination function.

Then, in a follow-up PR, we can discuss how to enhance OnlyValidArgs with ValidArgsFunction in order to support the kubectl example with aliases.

@umarcor umarcor changed the title Generalize ValidArgs: use it implicitly with any validator deprecate ExactValidArgs and test combinations of args validators Mar 27, 2022
@umarcor umarcor force-pushed the ValidArgs branch 2 times, most recently from 851f128 to 3fed561 Compare March 28, 2022 00:07
user_guide.md Outdated

Moreover, `MatchAll(pargs ...PositionalArgs)` enables combining existing checks with arbitrary other checks.
For instance, you want to check the ExactArgs length along with OnlyValidArgs to report an error if there are not
exactly N positional args OR if there are any positional args that are not in the `ValidArgs` field of `Command`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence is hard to parse for me. How about reversing it to something like:

"For instance, if you want to report an error if there are not exactly N positional args OR if there are any positional args that are not in the ValidArgs field of Command, you can call MatchAll on ExactArgs and OnlyValidArgs as shown below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Thanks for doing the detailed investigation @umarcor! I didn't know all these details.

So, the inconsistency was introduced in #765

Yeah, it looks like that PR was simply addressing the exact need of the author at the time.

I like the formality this PR introduces. I think it will make maintenance easier.
And the new tests are great.

@marckhouzam marckhouzam added the lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready label Mar 28, 2022
@github-actions github-actions bot added the size/XL Denotes a PR that exceeds 200 lines. Caution! label Mar 30, 2022
@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 30, 2022

I like the formality this PR introduces. I think it will make maintenance easier.
And the new tests are great.

This is the goal: https://github.com/umarcor/cobra/blob/args-subtests/args_test.go

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

github-actions bot commented May 3, 2022

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

github-actions bot commented May 5, 2022

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@umarcor
Copy link
Contributor Author

umarcor commented Jul 13, 2022

@marckhouzam can we please merge this? #1646 has been waiting for three months already...

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

@umarcor I had another pass at this one and there are couple of things I think we need to tweak, but then it will be good to go.

args.go Outdated
//
// Deprecated: use MatchAll(OnlyValidArgs, ExactArgs(n)) instead
func ExactValidArgs(n int) PositionalArgs {
return MatchAll(OnlyValidArgs, ExactArgs(n))
Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep the same error reported as now, I think we should inverse the two methods:
return MatchAll(ExactArgs(n), OnlyValidArgs)
That way if both conditions fail, it will be the wrong number of args reported, like is done currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b3982ec.

args_test.go Outdated
}

func TestExactValidArgs_WithInvalidArgs(t *testing.T) {
c := getCommand(ExactValidArgs(3), true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the test title, we should probably specify 2 args here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 83365ac.

args_test.go Outdated
}

func TestExactValidArgs_WithInvalidCount_WithInvalidArgs(t *testing.T) {
c := getCommand(ExactValidArgs(3), true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the test title, I believe we want to specify the wrong number of args and the wrong values for them.
So, we should probably specify 2 args here since we want the next line to be wrong and it specifies 3 args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4e31482.

args_test.go Outdated
func TestExactValidArgs_WithInvalidCount_WithInvalidArgs(t *testing.T) {
c := getCommand(ExactValidArgs(3), true)
_, err := executeCommand(c, "three", "a", "two")
validOnlyWithInvalidArgs(err, t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be exactArgsWithInvalidCount(err, t) as we should report invalid number first, when both conditions fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b3982ec.

args_test.go Outdated
noArgsWithArgs(err, t)
}

func TestNoArgs_WithValid_WithInvalidArgs(t *testing.T) {
c := getCommand(NoArgs, true)
_, err := executeCommand(c, "one")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the title of the test, the arg here should be "a". Currently this is the exact same test as the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6863083.

@marckhouzam marckhouzam added this to the 1.6.0 milestone Aug 28, 2022
@marckhouzam
Copy link
Collaborator

I've added this to the 1.6 milestone

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

1 similar comment
@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for your patience and effort @umarcor !

@marckhouzam marckhouzam merged commit 70e53f6 into spf13:main Sep 10, 2022
17 checks passed
@umarcor umarcor deleted the ValidArgs branch September 11, 2022 14:50
jimschubert added a commit to jimschubert/cobra that referenced this pull request Oct 3, 2022
* main: (39 commits)
  Add '--version' flag to Help output (spf13#1707)
  Expose ValidateRequiredFlags and ValidateFlagGroups (spf13#1760)
  Document option to hide the default completion cmd (spf13#1779)
  ci: add workflow_dispatch (spf13#1387)
  add missing license headers (spf13#1809)
  ci: use action/setup-go's cache (spf13#1783)
  Adjustments to documentation (spf13#1656)
  Rename Powershell completion tests (spf13#1803)
  Support for case-insensitive command names (spf13#1802)
  Deprecate ExactValidArgs() and test combinations of args validators (spf13#1643)
  Use correct stale action `exempt-` yaml keys (spf13#1800)
  With go 1.18, we must use go install for a binary (spf13#1726)
  Clarify SetContext documentation (spf13#1748)
  ci: test on Golang 1.19 (spf13#1782)
  fix: show flags that shadow parent persistent flag in child help (spf13#1776)
  Update gopkg.in/yaml.v2 to gopkg.in/yaml.v3 (spf13#1766)
  fix(bash-v2): activeHelp length check syntax (spf13#1762)
  fix: correct command path in see_also for YAML doc (spf13#1771)
  build(deps): bump github.com/inconshreveable/mousetrap (spf13#1774)
  docs: add zitadel to the list (spf13#1772)
  ...
kajes added a commit to appgate/sdpctl that referenced this pull request Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flags-args Changes to functionality around command line flags and args kind/feature A feature request for cobra; new or enhanced behavior lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready size/XL Denotes a PR that exceeds 200 lines. Caution!
Projects
None yet
2 participants