add ability to ignore unknown flags

[rajatjindal]

It will be nice if we can add ability to ignore unknown flags. This can be useful when writing wrappers on other commands where exposing all downstream flags is un-necessary.

[CLAassistant]

All committers have signed the CLA.

[rajatjindal]

requesting review from @eparis

[rajatjindal]

pinging @eparis again

[rajatjindal]

Pinging @eparis, please let me know if once a day ping is too much

[rajatjindal]

ping @spf13 @eparis

[rajatjindal]

ping @eparis

please continue to ignore if you are busy, i just want to make sure i don't forget about it.

[eparis]

How should this relate to the *OnError defines?
https://github.com/spf13/pflag/pull/160/files#diff-1fdeef6cfb1d74886a385d13094d6cbcL117
I'm specifically wondering how Missing is today an error, which means it would be handled by the *OnError defines, but after this patch we have multiple error handle things...

Can you think of a good way to rationalize this into a single error handling logic?

[rajatjindal]

Thanks a lot for the review @eparis

I see what you are saying.

looking at this: https://github.com/spf13/pflag/blob/master/flag.go#L1087

i think with ContinueOnError, it will stop parsing flags as soon as it encounter an unknown flag, but let user decide what do to with error

with IgnoreUnknownFlags, it will ignore unknown flag error encountered during parse while continuing to parse the other known flags. (it will still return error for other conditions).

If it make sense, I can add this as an "ErrorHandling" type. Please let me know

[eparis]

Do you think IgnoreUnknownFlags is a better API? I think it makes sense to me.

I'm not arguing that the "ErrorHandling" stuff is a good idea, it's just the thing we have. And I don't want to make a second semi-related thing if we can help it....

[rajatjindal]

if we add another ErrorHandling type as 'IgnoreUnknownFlags', what should be the behavior incase of any other error.

e.g. lets say user selected ErrorHandling to be of type IgnoreUnknownFlags, and flag parsing encountered a different error, what should we do with that error (panic, exit or continue?)

[rajatjindal]

sorry if felt that way, but i am asking for suggestions here :)

this library is a master piece, and i want to keep it that way (even after my ugly contributions)

[rajatjindal]

so kindly let me know what you think we should do here, and I will try to update the PR to reflect that.

[rajatjindal]

pinging @eparis

[eparis]

I'd like you to think about what a better Error handling interface would look like. I agree that having 3 new OnError defines is not the right thing to go. I'll take this PR, but would love to hear you thought if some sort of feature bit map or something makes better sense for our error handling. And if it does make sense, can this be the first thing the implements that new error aPI?

[eparis]

What if I passed --unknown=bob --next-flag. I think this will strip --next-flag, but I don't think it should.

[rajatjindal]

this function just strips value for unknown flag, the flag itself is not included in args.

for --unknown=value --next-flag ... args will be --next-flag ...

added the testcase to test this explicitly

[eparis]

I really think you need to know len(split) (from parseLongArg) and to use that. What if it was --unknown=value subcmd. You would strip subcmd which is really not what the user wants.

Clearly it is a heuristic to know what to strip and what not to strip for an 'unknown' but I think we want to be as careful as we can...

[rajatjindal]

oh yes, thats right. i would consider this a bug :)

fixed. thank you for pointing out.

[eparis]

I think this test need to cover the case where --unknown=value is followed by --known

[rajatjindal]

added the testcase, thanks

[rajatjindal]

thanks for the review @eparis

I have added the test case as suggested.

[eparis]

Convert this into some sort of generic "set" of error handling bools which we can grow as needed and I'll merge this PR.

[rajatjindal]

Will do that today, thanks

[rajatjindal]

thanks a lot for merging eparis

[vcastellm]

@rajatjindal I was waiting for this PR thanks for the effort! If I understand correctly, it strips all unknown flags passed, leaving known ones.

I need to create a wrapper command in the form: cmd subcmd systemcmd --systemcmd-flag

Where systemcmd is any executable given by the user.

What happens is that I'm loosing any given --systemcmd-flag passed. What is the goal of this PR if those flags are stripped?

[rajatjindal]

originally pflag will error out if it encounters any flag which was not configured by 'current cmd'. so running cmd subcmd systemcmd --systemcmd-flag, will result in 'unknown flag --systemcmd-flag'

The intention of this PR was to ignore those unknown flags during parsing (in other words strip) and keep the flags which are required by 'current cmd'.

do u have a use case where you want to consume systemcmd-flag in your 'current cmd'?

[vcastellm]

@rajatjindal I want to build a command wrapper similar to the bundle exec http://bundler.io/man/bundle-exec.1.html interface.

I need to preserve all flags passed to the command being wrapped.

Currently this is not possible to do with the previous pflag and it's still not possible with this PR.

[eparis]

I think it is possible, but now you have go back and process os.Args yourself inside Run()

what if we added the 'ignored' flags to the args array we pass to Run()

[vcastellm]

@eparis thanks for pointing that, this should workaround the problem.

I expected to find the "ignored" flags in the args passed to Run(), I could craft a PR with that, what do you think?

[eparis]

we'll see what @rajatjindal says about it. This API is new enough I don't mind 'breaking' it....

[rajatjindal]

I think we should probably include IgnoredFlags *flag.FlagSet field in command, that can be set when ignoredflags is on

Passing by default to run func may create prob again for user on what is child commands vs ignored flags

[vcastellm]

@rajatjindal but would that require the user to explicitly set IgnoredFlags in advance?

I would add IgnoredFlags []string

[rajatjindal]

Yeah, i think so, otherwise the flag parsing will fail on it.

I think []string make sense

[rajatjindal]

sorry i think i misread your comment. The user will just have to set a bool flag to ignore unknown flags, but ignored flags will be read and added to IgnoredFlags while parsing.

[eparis]

Making sure my thought was understood. I was saying that if we ignore a flag and arg (because the dev decided to set IgnoreUnknown) we should add those things to the args here: https://github.com/spf13/cobra/blob/master/command.go#L103

Today, I think, those args are just 'non-flag' args which were not consumed by cobra. If you set 'IgnoreUnknown' your program will start getting those 'ignored' things along with the non-flag args. Now it would be your program's problem to parse and handle those 'args'. How you decide to handle those are your problem...

[vcastellm]

I'm totally with @eparis, just unsure to be saying something nonsense, but passing everything back to the user in args makes sense to me.

[rajatjindal]

I think thats fine too. args it is then :)

[lucastheisen]

@rajatjindal , the last few messages in this discussion seem to indicate that any non-consumed flags should be left in the args array passed to Run. That is not the behavior that I am observing. I am currently using

	github.com/spf13/pflag v1.0.2 // indirect

I am writing a wrapper around kubectl logs:

var logsCmd = &cobra.Command{
	Use:   "logs [app name]",
	Short: "Fetch the logs of a container",
	Args:  cobra.MinimumNArgs(1),
	FParseErrWhitelist: cobra.FParseErrWhitelist{
		UnknownFlags: true,
	},
	RunE: func(cmd *cobra.Command, args []string) error {
		result, err := util.Runner{
			BufferStdout: true,
		}.Run("kubectl", []string{
			"get",
			"pod",
			"-l",
			fmt.Sprintf("app=%s", args[0]),
			"-o",
			"jsonpath={.items[0].metadata.name}",
		}...)
		if err != nil {
			return err
		}

		log.Debugf("running logs args [%v]", args)
		_, err = util.Runner{
			PrintStdout: true,
			PrintStderr: true,
		}.Run("kubectl",
			append([]string{
				"logs",
				result.Stdout.String(),
			}, args[1:]...)...)
		return err
	},
}

But when i use go run . logs foodb -f, the -f is not getting passed in args:

...
�[37mDEBU�[0m[0001] running logs args [[foodb]]
...

It appears your test code verifies that all the expected flags are parsed, and unexpected flags ignored, but it doesn't seem to be verifying that the unexpected args are present in the remaining arg list. Also, this ignore feature never made its way into the cobra documentation, it was a bit of a pita to figure this out. Might be a good idea to add it there.

[rajatjindal]

Hi

Sorry for the pain it caused

I am in middle of relocating right now, i can try to fix this in about 2 weeks or if you want to open a PR that will be great

[mtheck]

Totally agree, an ignored flag should have been kept in the argument list after flag parsing, because it is ignored. This is an important missing feature of this library which prevent developing a command wrapper (as said by @Victorcoder) that has its own "global" arguments.

However this features is implemented as a kind of error to be ignored and not flags to be ignored. So I don't think it is worthwhile breaking by changing this behavior. There should be a real "IgnoreUnknownFlags" feature.

[marcusljx]

Is there any update on the ignored flag being passed back to the remaining arg list?

[sethvargo]

FWIW, using -- works as people expect with no additional options, including this configuration.

var execCmd = &cobra.Command{
	Use:   "exec -- [subcommand]",
	Short: "Spawn a subprocess",
	Args: cobra.MinimumNArgs(1),
	Run:  execRun,
}

func execRun(_ *cobra.Command, args []string) {
	execCmd := args[0]
	execArgs := args[1:]
	# ...
}

I agree it's not the ideal UX, but it gets the job done:

$ mycli exec -- ls -alh

[duglin]

Is there any update on the ignored flag being passed back to the remaining arg list?

I can't seem to find a way to access the ignore flags - was this ever addressed?

[xmapst]

Is there any update on the ignored flag being passed back to the remaining arg list?

I can't seem to find a way to access the ignore flags - was this ever addressed?

cmd = &cobra.Command{
		......
		FParseErrWhitelist: cobra.FParseErrWhitelist{
			UnknownFlags: true,
		},
	}