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

Mandatory Flags not verified before Prerun. #655

Closed
amanhigh opened this issue Mar 26, 2018 · 8 comments
Closed

Mandatory Flags not verified before Prerun. #655

amanhigh opened this issue Mar 26, 2018 · 8 comments

Comments

@amanhigh
Copy link

Presence of Mandatory Flags is not verified before Prerun unlike Args Check which happens before Prerun.

Hence can't safely use Mandatory Flags in Prerun without additional manual checks.

Sample Code Demonstrating.

package main

import (
	"fmt"
	"github.com/spf13/cobra"
)

var testFlag = ""

var rootCmd = &cobra.Command{
	Use:   "test",
	Short: "Test Command",
	PreRun: func(cmd *cobra.Command, args []string) {
		fmt.Println("Value for Mandator TestFlag is " + testFlag)
	},
	Run: func(cmd *cobra.Command, args []string) {

	},
}

func init() {
	rootCmd.Flags().StringVarP(&testFlag, "test", "t", "Nothing", "Test Flag")
	rootCmd.MarkFlagRequired("test")
}

func main() {
	rootCmd.Execute()
}

Output
Value for Mandator TestFlag is Nothing
Error: required flag(s) "test" not set
Usage:
test [flags]

Flags:
-h, --help help for test
-t, --test string Test Flag (default "Nothing")

@mattthym
Copy link

#747 Issues are related

@kasvith
Copy link

kasvith commented Jun 19, 2019

I recently got into same error

@relloyd
Copy link

relloyd commented Aug 12, 2019

I have a similar issue in that the configured default values for flags are not set if they are missing from the CLI. Parsing fails with errors like the one above, "Error: required flag(s) "test" not set". I would have expected the default value to be assumed for the flag if it's missing from the CLI.

I have to call Flags.Set(<name>,<default value>) even though I created the flag with a default value up front.

Have I missed something obvious?

@AdamSLevy
Copy link

This is old but still persists in the latest release. Marking a flag as required is effectively useless if you intend to use the flag value in the PreRun functions.

@github-actions
Copy link

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

@tasdikrahman
Copy link

Hey folks, was wondering if this is by design (apologies if I have missed another issue in case it discusses the same). I came across this issue recently.

To circumvent this issue, moved the flow added in PreRun to the Run flow, for the mandatory flags set to be checked.

@johnSchnake
Copy link
Collaborator

Yes, I think this is a duplicate issue but also one that comes up often.

The flag parsing doesnt happen until later in the flow and I think this is by design (it was always in that position, prerun wasn't added later or anything). This allows you to see/alter flag data/settings before flag validation would takeover and potentially error out.

There was another ticket that I think should be taken up which is simply to add another hook after flags parse but before run for clarity. That way you can more clearly understand where to put the logic that needs all the flag info but may be before the core 'run' logic.

Seeing as how there are multiple other issues of a similar vein and there are workarounds given in this thread, I'm going to go ahead and close this issue. Feel free to reopen if you think this is done in error.

@michaelajr
Copy link

It makes sense to me that flag validation would happen after PersistentPreRun. That is where we can manipulate the flags if needed. I use the PersistentPreRun hook to work around spf13/viper#397.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants