fix flag include problem #465

magicwang-cn · 2018-11-30T14:20:23Z

k8s-ci-robot · 2018-11-30T14:20:25Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment 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 sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

magicwang-cn · 2018-11-30T14:37:07Z

@font @onyiny-ang

marun · 2018-12-01T00:37:01Z

cmd/controller-manager/app/controller-manager.go

@@ -58,7 +59,6 @@ member clusters and does the necessary reconciliation`,
 			if verFlag {
 				os.Exit(0)
 			}
-			PrintFlags(cmd.Flags())


Why does this need to be removed?

this has already been print in the "k8s.io/apiserver/pkg/util/flag", see this

marun · 2018-12-01T00:37:17Z

cmd/controller-manager/app/controller-manager.go

-	opts.AddFlags(cmd.Flags())
-	cmd.Flags().BoolVar(&verFlag, "version", false, "Prints the Version info of controller-manager")
+	opts.AddFlags(pflag.CommandLine)
+	pflag.CommandLine.BoolVar(&verFlag, "version", false, "Prints the Version info of controller-manager")


Why is this line changing?

two reasons:

pflag.CommandLine is one FlagSet obj, see this, and cmd.Flags() will new another FlagSet obj., see this

and "k8s.io/apiserver/pkg/util/flag" chooses pflag.CommandLine as default, see this

but, if you don't want to use "k8s.io/apiserver/pkg/util/flag", there will be another way:

import "flag" opts.AddFlags(cmd.Flags()) cmd.Flags().AddGoFlagSet(flag.CommandLine) cmd.Flags().BoolVar(&verFlag, "version", false, "Prints the Version info of controller-manager")

marun · 2018-12-01T00:40:43Z

@magicwang-cn CI is failing to pass for a deployed control plane. Have you tested this to work locally (i.e. by deploying and running e2e tests as per the developer guide)?

magicwang-cn · 2018-12-01T03:43:07Z

@marun oh, sorry, i will check this :)

magicwang-cn · 2018-12-01T06:53:07Z

/retest

marun · 2018-12-01T06:55:17Z

The ci failure is not transient. Are you running tests locally?

magicwang-cn · 2018-12-01T08:12:21Z

sorry, i am doing the test in my local environment now. because my environment is under proxy, so download package is a problem. please wait...

magicwang-cn · 2018-12-01T09:29:49Z

@marun Unfortunately， (kind) command is not support to create cluster when host is behind a proxy, see this issue, i will do the work in another environment later.

marun · 2018-12-03T18:28:04Z

@magicwang-cn You don't have to replicate the environment exactly, and it should be possible to replicate the failure by deploying 2 clusters with minikube (as per the user docs) running the e2e tests against that (as per the dev docs).

magicwang-cn · 2018-12-04T03:17:41Z

@marun yeah, i have found the problem(flag has been redefined):

/root/controller-manager flag redefined: install-crds
panic: /root/controller-manager flag redefined: install-crds

goroutine 1 [running]:
github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag.(*FlagSet).AddFlag(0xc0000aa000, 0xc0001d60a0)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag/flag.go:836 
+0x870
github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag.(*FlagSet).VarPF(0xc0000aa000, 0x15be7a0, 0xc00
01bfbd9, 0x140863a, 0xc, 0x0, 0x0, 0x1446a77, 0x3a, 0x20)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag/flag.go:819 
+0x10b
github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag.(*FlagSet).BoolVarP(0xc0000aa000, 0xc0001bfbd9,
 0x140863a, 0xc, 0x0, 0x0, 0x21cbb01, 0x1446a77, 0x3a)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag/bool.go:55 +
0x97
github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag.(*FlagSet).BoolVar(0xc0000aa000, 0xc0001bfbd9, 
0x140863a, 0xc, 0x40c001, 0x1446a77, 0x3a)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/vendor/github.com/spf13/pflag/bool.go:50 +
0x74
github.com/kubernetes-sigs/federation-v2/cmd/controller-manager/app/options.(*Options).AddFlags(0xc0001bfbc0, 0xc0000a
a000)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/cmd/controller-manager/app/options/options
.go:42 +0x6e
github.com/kubernetes-sigs/federation-v2/cmd/controller-manager/app.NewControllerManagerCommand(0x454f90)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/cmd/controller-manager/app/controller-mana
ger.go:70 +0x153
main.NewHyperFedCommand.func1(0x10)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/cmd/hyperfed/main.go:78 +0x22
main.commandFor(0x7ffd78facdfe, 0x12, 0xc0000c4780, 0xc0004435f0, 0x2, 0x2, 0xc0005d9f88)
/home/wangbo/workspace/src/github.com/kubernetes-sigs/federation-v2/cmd/hyperfed/main.go:62 +0x6b

see this and this, if use (pflag.CommandLine), there will be redefined.

magicwang-cn · 2018-12-05T01:37:59Z

@marun it's ok now

marun · 2018-12-05T02:32:36Z

cmd/controller-manager/app/controller-manager.go

@@ -67,9 +69,18 @@ member clusters and does the necessary reconciliation`,
 		},
 	}

+	// Add the command line flags from other dependencies, but do not


What dependencies, and why is normalization required?

dependencies like glog, kubebulider, etc. i will add to the comment.

normalization just convert '_' to '-', does this useless?

I removed those lines and things still worked, so I think they should be removed. YAGNI (you ain't gonna need it) is a general principle of software development to avoid doing work until it is actually needed.

marun · 2018-12-06T07:03:34Z

Thank you!

/approve
/lgtm

k8s-ci-robot · 2018-12-06T07:03:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: magicwang-cn, marun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [marun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 30, 2018

k8s-ci-robot requested review from font and onyiny-ang November 30, 2018 14:20

marun reviewed Dec 1, 2018

View reviewed changes

magicwang-cn force-pushed the bugfix-flag branch from aa0f99d to a26ab8d Compare December 1, 2018 06:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 1, 2018

magicwang-cn force-pushed the bugfix-flag branch from a26ab8d to e4e051f Compare December 4, 2018 03:10

magicwang-cn force-pushed the bugfix-flag branch from e4e051f to 37e6842 Compare December 4, 2018 04:58

marun reviewed Dec 5, 2018

View reviewed changes

magicwang-cn force-pushed the bugfix-flag branch from 37e6842 to 6024c86 Compare December 6, 2018 01:36

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 6, 2018

fix flag include problem

312decd

magicwang-cn force-pushed the bugfix-flag branch from 6024c86 to 312decd Compare December 6, 2018 01:37

k8s-ci-robot assigned marun Dec 6, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2018

k8s-ci-robot merged commit f4eb1db into kubernetes-retired:master Dec 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix flag include problem #465

fix flag include problem #465

magicwang-cn commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

magicwang-cn commented Nov 30, 2018

marun Dec 1, 2018

magicwang-cn Dec 1, 2018

marun Dec 1, 2018

magicwang-cn Dec 1, 2018 •

edited

Loading

marun commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

marun commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

marun commented Dec 3, 2018

magicwang-cn commented Dec 4, 2018 •

edited

Loading

magicwang-cn commented Dec 5, 2018

marun Dec 5, 2018

magicwang-cn Dec 5, 2018

marun Dec 5, 2018

magicwang-cn Dec 6, 2018

marun commented Dec 6, 2018

k8s-ci-robot commented Dec 6, 2018

fix flag include problem #465

fix flag include problem #465

Conversation

magicwang-cn commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

magicwang-cn commented Nov 30, 2018

marun Dec 1, 2018

Choose a reason for hiding this comment

magicwang-cn Dec 1, 2018

Choose a reason for hiding this comment

marun Dec 1, 2018

Choose a reason for hiding this comment

magicwang-cn Dec 1, 2018 • edited Loading

Choose a reason for hiding this comment

marun commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

marun commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

magicwang-cn commented Dec 1, 2018

marun commented Dec 3, 2018

magicwang-cn commented Dec 4, 2018 • edited Loading

magicwang-cn commented Dec 5, 2018

marun Dec 5, 2018

Choose a reason for hiding this comment

magicwang-cn Dec 5, 2018

Choose a reason for hiding this comment

marun Dec 5, 2018

Choose a reason for hiding this comment

magicwang-cn Dec 6, 2018

Choose a reason for hiding this comment

marun commented Dec 6, 2018

k8s-ci-robot commented Dec 6, 2018

magicwang-cn Dec 1, 2018 •

edited

Loading

magicwang-cn commented Dec 4, 2018 •

edited

Loading