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

Support TryAddFlagSet and EnvVars and AddFlag #333

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

robberphex
Copy link

  1. Add func TryAddFlag{,Set}, which add flag{Set} and ignore conflicts.
  2. Support EnvVars, and show env name in help message.
  3. Support AddFlag.
    originally, AddFlag can accept a pflag.Flag, but user cannot build Value field for pflag.Flag. So I rename pflag.newStringValue to pflag.NewStringValue to make AddFlag usable, for example:
  child.Flags().AddFlag(
  	&pflag.Flag{
  		Name:     "testArg",
  		Usage:    "usage",
  		EnvVars:  []string{"TEST_ARG", "TEST_ARG1"},
  		Value:    pflag.NewStringValue("default", new(string)),
  	},
  )

This was referenced Oct 11, 2021
@fahedouch
Copy link

@robberphex

It looks like it is not showing correctly the default value

-l, --leaderElection        Use leader election for the conroller manager. (default ) [$LEADER_ELECTION]

Extract from my source code

	leaderElection := new(bool)
	rootCmd.PersistentFlags().AddFlag(&pflag.Flag{
		Name:      "leaderElection",
		Shorthand: "l",
		Usage:     "Use leader election for the conroller manager.",
		EnvVars:   []string{"LEADER_ELECTION"},
		Value:     pflag.NewBoolValue(false, leaderElection),
	}

sval = envVal
} else {
sval = flag.Value.String()
}

Choose a reason for hiding this comment

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

When the both the flag value and the env var are specified, the flag value should be prioritized over the env value

containerd/nerdctl#495

@AkihiroSuda
Copy link

Could you update the commit messages to be more descriptive?

@fahedouch
Copy link

@robberphex do you plan to move forward on this?

@AkihiroSuda
Copy link

nerdctl now uses unforked cobra, so this PR is no longer needed from nerdctl perspective.
containerd/nerdctl#524

@fahedouch
Copy link

thanks @AkihiroSuda

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

Successfully merging this pull request may close these issues.

3 participants