This is causing a nil dereference in hack/test-cmd.sh. I think the issue is that the validate flag is added to the flagset of each command but not to factory.flags. Can probably fix by adding in BindFlags.

I don't know if that will work, as that would redeclare the same set of flags twice; can't remember whether that will overwrite or panic. I would suggest moving the declaration of these flags

flags.BoolVar(&f.clients.matchVersion, FlagMatchBinaryVersion, false, "Require server version to match client version")
flags.Bool("validate", false, "If true, use a schema to validate the input before sending it")

from f.BindFlags to NewFactory. That way they are added to f.flags and any command flagsets passed to f.BindFlags.

That's what I thought at first. But, wouldn't that be a bit confusing? setting a flag earlier so it doesn't cause a bug few calls later. It's a global flag design issue though.

I don't know that every command needs to validate. Get, describe, and a few others definitely do not.

On Mar 15, 2015, at 7:09 PM, Jeff Lowdermilk notifications@github.com wrote:

In pkg/kubectl/cmd/cmd.go:

        client, err := clients.ClientForVersion(mapping.APIVersion)
        if err != nil {
            return nil, err
        }
        return kubectl.ReaperFor(mapping.Kind, client)
    },

•   Validator: func(cmd *cobra.Command) (validation.Schema, error) {
  

•       if cmdutil.GetFlagBool(cmd, "validate") {
  

•   Validator: func() (validation.Schema, error) {
  

•       if flags.Lookup("validate").Value.String() == "true" {
  

  This is causing a nil dereference in hack/test-cmd.sh. I think the issue is that the validate flag is added to the flagset of each command but not to factory.flags. Can probably fix by adding in BindFlags.

—
Reply to this email directly or view it on GitHub.

@TamerTas, it's also reasonable to keep global flag declarations in bind flags and just declare them into f.flags before they are added to the passed-in flagset.

I don't know that every command needs to validate. Get, describe, and a few others definitely do not.

I agree, but changing global flag behavior seemed out of scope for this PR.

@jlowdermilk I just added a nil reference check before validation flag dereference.

validationFlag will always be nil, because the validate flag isn't being added to f.flags, only to the command flagset. If we aren't adding it in, we might as well remove this if block, but I don't know what ClientForVersion checks.

@jlowdermilk Then, we either remove it or add an
if validationFlag == nil { f.flags.Bool("validate") } block to BindFlags method.
A better solution would extend the scope of this pull request. But, I think flag system needs an overhaul.

Fine with me if you want to extend the scope of the PR to better handle global flags. If you just want this to be cmd refactoring, we should preserve the existing behavior with respect to validate, and that pretty much requires adding it to f.flags.

@jlowdermilk I've added the validate flag to f.flags as a temporary solution. I'm currently investigating a replacement for flags as configuration stored in etcd #5582.