Introduce --kubeconfig instead of --config #18806

soltysh · 2018-03-02T14:11:25Z

deads2k · 2018-03-02T14:20:39Z

pkg/oc/admin/diagnostics/config.go

@@ -32,7 +32,7 @@ func (o DiagnosticsOptions) detectClientConfig() (expected bool, detected bool)
 		return false, false
 	}
 	o.Logger().Notice("CED2011", "Determining if client configuration exists for client/cluster diagnostics")
-	confFlagName := config.OpenShiftConfigFlagName


this looks breaking. They're doing odd things and you have to tolerate both right? Add a test that would have caught this.

@sosiouxme I'll defer to you. We're trying to switch from --config to --kubeconfig and deprecated the former, does this fix look right?

@sosiouxme I'll defer to you. We're trying to switch from --config to --kubeconfig and deprecated the former, does this fix look right?

Does --config still work?

--config is still accepted but is ignored, aside from deprecation notice. That's a little surprising as this is only looking at the value of the flag, which (since both new and old have the same target) I expected to be the same regardless of which flag was looked up. But apparently flags store the value separately from the target.

So the old flag remains, but has no effect. The code needs to be updated to either look at both flags or look at the shared target.

--config is still accepted but is ignored, aside from deprecation notice.

@soltysh that won't work. Deprecated doesn't mean broken. The code needs to accept both.

that means it is probably broken elsewhere too.

I would actually not be surprised if this is the only place that looks at the flag itself as opposed to the value it sets. And I'd be happy to have it look at the value instead if I could figure out how to retrieve it... digging.

deads2k · 2018-03-02T14:20:58Z

pkg/oc/admin/diagnostics/diagnostics.go

@@ -245,7 +245,7 @@ func (o *DiagnosticsOptions) bindCommonFlags(flags *flag.FlagSet) {
 func (o *DiagnosticsOptions) bindClientFlags(flags *flag.FlagSet) {
 	o.ClientFlags = flag.NewFlagSet("client", flag.ContinueOnError) // hide the extensive set of client flags
 	o.Factory = osclientcmd.New(o.ClientFlags)                      // that would otherwise be added to this command
-	flags.AddFlag(o.ClientFlags.Lookup(config.OpenShiftConfigFlagName))
+	flags.AddFlag(o.ClientFlags.Lookup(kclientcmd.RecommendedConfigPathFlag))


looks breaking.

@sosiouxme same question

deads2k · 2018-03-02T14:21:06Z

pkg/oc/admin/diagnostics/diagnostics/network/setup.go

@@ -279,7 +279,7 @@ func (d *NetworkDiagnostic) getKubeConfig() ([]byte, error) {
 	// 2. Default admin config paths
 	// 3. Default openshift client config search paths
 	paths := []string{}
-	paths = append(paths, d.ClientFlags.Lookup(config.OpenShiftConfigFlagName).Value.String())
+	paths = append(paths, d.ClientFlags.Lookup(kclientcmd.RecommendedConfigPathFlag).Value.String())


looks breaking.

@sosiouxme same question

deads2k · 2018-03-02T14:22:16Z

pkg/oc/cli/cmd/wrappers.go

-
+		GlobalFile:        cmdconfig.RecommendedHomeFile,
+		EnvVar:            cmdconfig.OpenShiftConfigPathEnvVar,
+		ExplicitFileFlag:  kclientcmd.RecommendedConfigPathFlag,


Looks breaking. Patches required?

Breaking as in, this will be the default flag asking for kubeconfig. Isn't that what we need to to if we want to deprecate --config in 3.10?

Breaking as in, this will be the default flag asking for kubeconfig. Isn't that what we need to to if we want to deprecate --config in 3.10?

Does it respect --config at all anymore?

deads2k · 2018-03-02T14:23:00Z

test/integration/login_test.go

@@ -149,7 +148,7 @@ func newLoginOptions(server string, username string, password string, insecure b
 func defaultClientConfig(flags *pflag.FlagSet) clientcmd.ClientConfig {
 	loadingRules := &clientcmd.ClientConfigLoadingRules{ExplicitPath: ""}

-	flags.StringVar(&loadingRules.ExplicitPath, config.OpenShiftConfigFlagName, "", "Path to the config file to use for CLI requests.")
+	flags.StringVar(&loadingRules.ExplicitPath, clientcmd.RecommendedConfigPathFlag, "", "Path to the kubeconfig file to use for CLI requests.")


This somehow isn't breaking?

deads2k · 2018-05-08T15:13:05Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/factory_client_access.go

-	} else {
-		flags.StringVar(&loadingRules.ExplicitPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests.")
-	}
+	flags.StringVar(&loadingRules.ExplicitPath, clientcmd.OpenShiftKubeConfigFlagName, "", "Path to the kubeconfig file to use for CLI requests.")


This needs to be conditional. When running as kubectl we don't want this flag available.

deads2k · 2018-05-08T15:13:28Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/util/factory_client_access.go

-	} else {
-		flags.StringVar(&loadingRules.ExplicitPath, "kubeconfig", "", "Path to the kubeconfig file to use for CLI requests.")
-	}
+	flags.StringVar(&loadingRules.ExplicitPath, clientcmd.OpenShiftKubeConfigFlagName, "", "Path to the kubeconfig file to use for CLI requests.")


does binding the same flag twice actually work? Which one wins?

yes, multiple flags can bind to the same underlying value. last one parsed wins, just like if you specify the same flag multiple times:

oc --config foo.kubeconfig --config bar.kubeconfig get pods error: stat bar.kubeconfig: no such file or directory

deads2k · 2018-05-08T15:15:57Z

pkg/oc/cli/config/loader.go

 }

 func NewPathOptionsWithConfig(configPath string) *kclientcmd.PathOptions {
 	return &kclientcmd.PathOptions{
 		GlobalFile: kclientcmd.RecommendedHomeFile,

 		EnvVar:           kclientcmd.RecommendedConfigPathEnvVar,
-		ExplicitFileFlag: kclientcmd.OpenShiftKubeConfigFlagName,
+		ExplicitFileFlag: kclientcmd.RecommendedConfigPathFlag,


This looks broken. Please explicitly check both flags with oc login and oc project

I've added test to login.sh checking both login and project.

deads2k · 2018-05-08T15:16:31Z

pkg/oc/cli/config/loader.go

@@ -8,15 +8,15 @@ import (
 )

 func NewPathOptions(cmd *cobra.Command) *kclientcmd.PathOptions {
-	return NewPathOptionsWithConfig(kcmdutil.GetFlagString(cmd, kclientcmd.OpenShiftKubeConfigFlagName))
+	return NewPathOptionsWithConfig(kcmdutil.GetFlagString(cmd, kclientcmd.RecommendedConfigPathFlag))
 }

 func NewPathOptionsWithConfig(configPath string) *kclientcmd.PathOptions {
 	return &kclientcmd.PathOptions{


I was expecting a patch in this type that allowed the specification of two explicit flags.

soltysh · 2018-05-10T10:31:24Z

/refresh

openshift-ci-robot · 2018-05-10T13:52:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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:

~~contrib/completions/OWNERS~~ [soltysh]
hack/OWNERS
pkg/cmd/infra/router/OWNERS
~~pkg/oc/OWNERS~~ [soltysh]
~~test/cmd/OWNERS~~ [soltysh]
test/integration/OWNERS
vendor/k8s.io/kubernetes/pkg/OWNERS
vendor/k8s.io/kubernetes/staging/src/k8s.io/client-go/OWNERS

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

soltysh · 2018-05-10T14:41:31Z

Ha, my problem was with wrong apostrophes in login.sh let's wait for the test results now.

…errors

soltysh · 2018-05-22T14:23:58Z

/retest

openshift-ci-robot · 2018-05-22T15:05:01Z

@soltysh: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_networking_minimal	`7c6f2c2`	link	`/test extended_networking_minimal`
ci/openshift-jenkins/end_to_end	`21b0277`	link	`/test end_to_end`
ci/openshift-jenkins/extended_conformance_install	`21b0277`	link	`/test extended_conformance_install`
ci/openshift-jenkins/gcp	`21b0277`	link	`/test gcp`
ci/openshift-jenkins/cmd	`21b0277`	link	`/test cmd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

openshift-bot · 2018-06-20T20:01:45Z

@soltysh: PR needs rebase.

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.

soltysh · 2018-08-22T13:56:02Z

Closing in favor of #20721.

openshift-ci-robot assigned deads2k and juanvallejo Mar 2, 2018

openshift-ci-robot requested review from liggitt and mfojtik March 2, 2018 14:11

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 2, 2018

deads2k reviewed Mar 2, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2018

soltysh force-pushed the kubeconfig branch from 7c6f2c2 to 436f979 Compare May 8, 2018 15:06

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2018

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 8, 2018

deads2k reviewed May 8, 2018

View reviewed changes

soltysh force-pushed the kubeconfig branch from 436f979 to a4d63e2 Compare May 10, 2018 13:51

soltysh force-pushed the kubeconfig branch from a4d63e2 to ebdb2b1 Compare May 10, 2018 14:41

soltysh force-pushed the kubeconfig branch from ebdb2b1 to 450d5b6 Compare May 18, 2018 13:52

UPSTREAM: <carry>: allow oc kubeconfig loading to have our flags and …

eeb19e9

…errors

soltysh force-pushed the kubeconfig branch from 450d5b6 to 7c00a9e Compare May 22, 2018 12:35

soltysh added 2 commits May 22, 2018 15:54

Introduce --kubeconfig instead of --config

e876021

Generated

21b0277

soltysh force-pushed the kubeconfig branch from 7c00a9e to 21b0277 Compare May 22, 2018 13:55

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018

soltysh closed this Aug 22, 2018

soltysh deleted the kubeconfig branch August 22, 2018 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce --kubeconfig instead of --config #18806

Introduce --kubeconfig instead of --config #18806

soltysh commented Mar 2, 2018

deads2k Mar 2, 2018

soltysh Mar 2, 2018

deads2k Mar 2, 2018

sosiouxme Mar 2, 2018

deads2k Mar 2, 2018

sosiouxme Mar 2, 2018

deads2k Mar 2, 2018

soltysh Mar 2, 2018

deads2k Mar 2, 2018

soltysh Mar 2, 2018

deads2k Mar 2, 2018

soltysh Mar 2, 2018

deads2k Mar 2, 2018

deads2k Mar 2, 2018

deads2k May 8, 2018

soltysh May 10, 2018

deads2k May 8, 2018

liggitt May 8, 2018

deads2k May 8, 2018

soltysh May 10, 2018

deads2k May 8, 2018

soltysh May 10, 2018

soltysh commented May 10, 2018

openshift-ci-robot commented May 10, 2018

soltysh commented May 10, 2018

soltysh commented May 22, 2018

openshift-ci-robot commented May 22, 2018

openshift-bot commented Jun 20, 2018

soltysh commented Aug 22, 2018

Introduce --kubeconfig instead of --config #18806

Introduce --kubeconfig instead of --config #18806

Conversation

soltysh commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented May 10, 2018

openshift-ci-robot commented May 10, 2018

soltysh commented May 10, 2018

soltysh commented May 22, 2018

openshift-ci-robot commented May 22, 2018

openshift-bot commented Jun 20, 2018

soltysh commented Aug 22, 2018