Warns the user for deprecated flags that use _ separators #8026

andronat · 2015-05-10T01:34:16Z

Fix for #7478

Also gets rid of "intermediate" representation of flags with . to enforce consistent flag name format throughout the project.

i.e. Formats like flag_f, flag-f and flag.f were all valid in the source code, now we throw a warning if a developer uses the format of flag_f and flag.f is invalid.

googlebot · 2015-05-10T01:34:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

andronat · 2015-05-10T01:39:37Z

I signed it!

googlebot · 2015-05-10T01:39:39Z

CLAs look good, thanks!

j3ffml · 2015-05-11T19:29:59Z

Thanks for the PR!

LGTM, but I know @smarterclayton has expressed distaste with glog, so I'll let him comment.

smarterclayton · 2015-05-11T19:32:36Z

It's fine for now - we need to go through and figure out what our log story is for all CLI commands anyway so this doesn't make it any worse.

andronat · 2015-05-11T19:56:00Z

Don't merge yet, sorry but I just found that it is not working with all the flags.

And I also have a situation that I am not sure on how to handle it. There are flags that are coming from other packages that are written with _. e.g. log_dir and log_backtrace_at from the glog package. How am I suppose to handle them? Sending a depreciation warning I think is useless on the situation.

j3ffml · 2015-05-11T20:25:40Z

I agree a deprecation warning on flags merged from other packages doesn't make sense. How we handle depends on whether we plan to continue using glog in kubectl or not. If yes, then we can't avoid having a mix of _ and - flags. @ghodss, thoughts?

andronat · 2015-05-11T21:10:59Z

Maybe we can "shadow" them? spf13/pflag#23

j3ffml · 2015-05-11T21:24:43Z

I think a better path is to do the translation ourselves when we import the flags into the kubectl command flagset here; see also AddFlagSetToPFlagSet . Doing the translation inside pflag seems wrong to me.

ghodss · 2015-05-11T21:35:00Z

@jlowdermilk's solution sounds great to me. The alternative is keeping a whitelist inside WordSepNormalizeFunc, which sounds worse.

andronat · 2015-05-11T21:46:15Z

Ok, so I will translate all the persistent flags and then I will enable the _ warning messages

andronat · 2015-05-19T17:13:48Z

spf13/pflag#23 and spf13/cobra#110 are merged please review.

/cc @ghodss

j3ffml · 2015-05-20T22:17:04Z

Looks almost ready. Can you rebase to 2 commits (1 to bump cobra/pflag, 1 with your changes)? Also, you'll need to run hack/run-gendocs.sh as the auto-generated kubectl docs have changed now that inherited flags are - separated. You will actually get a presubmit check telling to run gendocs, if you install the hooks.

andronat · 2015-05-20T23:28:23Z

Done, sorry for the docs. I added the hooks now

j3ffml · 2015-05-21T00:27:57Z

The cobra bump picked up spf13/cobra#109, which is causing the default GenMarkdownTree to prepend the filename to each generated md file, which we do not want.

If you don't mind waiting a bit for spf13/cobra#112 to be merged, and re-updating cobra that would be simplest. Otherwise, quick fix is to replace this line with

GenMarkdownTreeCustom(kubectl, outDir, func(s string) string { return "" }, func(s string) string { return s })

and leave a todo to update once spf13/cobra#112 is in.

eparis · 2015-05-21T03:43:57Z

@jlowdermilk merged fix in cobra.

j3ffml · 2015-05-21T03:52:21Z

Thanks @eparis! @andronat, if you update cobra and rerun hack/run-gendocs.sh tests should be passing.

eparis · 2015-05-21T03:53:01Z

pkg/util/pflag_import.go

 func AddFlagSetToPFlagSet(fsIn *flag.FlagSet, fsOut *pflag.FlagSet) {
 	fsIn.VisitAll(func(f *flag.Flag) {
 		addFlagToPFlagSet(f, fsOut)
 	})
 }

-// Add all of the top level 'flag' package flags to the top level 'pflag' flags.
+// AddAllFlagsToPFlags adds all of the top level 'flag' package flags to the top level 'pflag' flags.
 func AddAllFlagsToPFlags() {
 	AddFlagSetToPFlagSet(flag.CommandLine, pflag.CommandLine)
 }


Is this even needed, after spf13/cobra#75

I know it's not the point of your PR, but how much of pflag_import.go can we rip out?

Something still isn't quite right around this stuff. ignore me.

Actually I was also trying to get rid of those funcs there. The problem is that cobra is merging flag command line flags but is not working for the pflag global flags. Also an other problem is the order and "when" each of those merges are taking place. This is actually an issue of cobra and I might work on that at some point: spf13/cobra#44

Agree, we should work on this in cobra.

j3ffml · 2015-05-21T16:07:31Z

lgtm

andronat · 2015-05-21T21:38:44Z

Rebased to resolve conflicts

eparis · 2015-05-21T21:46:05Z

LGTM

dchen1107 · 2015-05-22T17:18:48Z

You have to rebase, sorry for this.

andronat · 2015-05-23T02:31:08Z

Done

k8s-bot · 2015-05-24T12:19:07Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

also renames all global _ flags to -

andronat · 2015-05-27T09:28:36Z

any news on that? It is ready to merge, I rebased it to latest master

eparis · 2015-05-27T13:40:19Z

I haven't tested it, but does using --flag_name= on the command line give a warning, or does it just accept things silently?

andronat · 2015-05-27T14:33:46Z

It is like that:

kubectl --no_headers=true get cs
W0527 14:31:51.039177     373 flags.go:36] no_headers is DEPRECATED and will be removed in a future version. Use no-headers instead.
controller-manager   Healthy   ok        nil
scheduler            Healthy   ok        nil

The same applies to all flags. It works for all cases but prints a warning

thockin · 2015-05-27T15:36:07Z

Are you guys ready to go with this - it is a substantial change, so I want to get it off the books one way or another.

eparis · 2015-05-27T15:37:20Z

LGTM

eparis · 2015-05-27T15:45:18Z

(but it's not assigned to me, so I'm not adding the label)

j3ffml · 2015-05-27T15:55:29Z

Still LGTM

thockin · 2015-05-27T16:30:16Z

fire in the hole

Warns the user for deprecated flags that use _ separators

googlebot added the cla: no label May 10, 2015

googlebot added cla: yes and removed cla: no labels May 10, 2015

zmerlynn assigned j3ffml May 10, 2015

andronat closed this May 19, 2015

andronat reopened this May 19, 2015

eparis reviewed May 21, 2015
View reviewed changes

j3ffml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2015

andronat closed this May 24, 2015

andronat reopened this May 24, 2015

andronat changed the title ~~Fixes GoogleCloudPlatform/kubernetes#7478~~ Warns the user for deprecated flags that use _ separators May 24, 2015

andronat added 2 commits May 27, 2015 11:23

updated cobra and pflag

a1ea3df

Warn for every _ flag user or cmd defined

5eae237

also renames all global _ flags to -

thockin added a commit that referenced this pull request May 27, 2015

Merge pull request #8026 from andronat/fix_7478

b9bfce4

Warns the user for deprecated flags that use _ separators

thockin merged commit b9bfce4 into kubernetes:master May 27, 2015

andronat deleted the fix_7478 branch May 27, 2015 17:05

eparis mentioned this pull request Jun 3, 2015

gendocs is demanding I regenerate docs, and is changing - to . in flag names. #9173

Closed

ghodss mentioned this pull request Jun 3, 2015

Migrate command line flags from a mix of _ and - to just - #7478

Closed

andronat unassigned j3ffml Aug 12, 2015

Warns the user for deprecated flags that use _ separators #8026

Warns the user for deprecated flags that use _ separators #8026

Conversation

andronat commented May 10, 2015

googlebot commented May 10, 2015

andronat commented May 10, 2015

googlebot commented May 10, 2015

j3ffml commented May 11, 2015

smarterclayton commented May 11, 2015

andronat commented May 11, 2015

j3ffml commented May 11, 2015

andronat commented May 11, 2015

j3ffml commented May 11, 2015

ghodss commented May 11, 2015

andronat commented May 11, 2015

andronat commented May 19, 2015

j3ffml commented May 20, 2015

andronat commented May 20, 2015

j3ffml commented May 21, 2015

eparis commented May 21, 2015

j3ffml commented May 21, 2015

eparis May 21, 2015

Choose a reason for hiding this comment

eparis May 21, 2015

Choose a reason for hiding this comment

andronat May 21, 2015

Choose a reason for hiding this comment

eparis May 21, 2015

Choose a reason for hiding this comment

j3ffml commented May 21, 2015

andronat commented May 21, 2015

eparis commented May 21, 2015

dchen1107 commented May 22, 2015

andronat commented May 23, 2015

k8s-bot commented May 24, 2015

andronat commented May 27, 2015

eparis commented May 27, 2015

andronat commented May 27, 2015

thockin commented May 27, 2015

eparis commented May 27, 2015

eparis commented May 27, 2015

j3ffml commented May 27, 2015

thockin commented May 27, 2015