Support reading KubeConfig from KUBECONFIG env var #293

aliok · 2020-02-12T10:57:25Z

Fixes #292

Proposed Changes

Backwards compatible change
Only use KUBECONFIG if the command line argument is empty

Release Note

NONE

Same PR for eventing operator: knative/eventing-operator#100

knative-prow-robot

@aliok: 0 warnings.

Details

In response to this:

Fixes #292

Proposed Changes

Backwards compatible change

Only use KUBECONFIG if the command line argument is empty

Release Note
NONE
Same PR for eventing operator: knative/eventing-operator#100

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.

knative-prow-robot · 2020-02-12T10:57:40Z

Hi @aliok. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

markusthoemmes · 2020-02-12T12:29:30Z

pkg/reconciler/knativeserving/controller.go

 	recursive  = flag.Bool("recursive", false, "If filename is a directory, process all manifests recursively")
 	MasterURL  = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
-	Kubeconfig = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
+	Kubeconfig = flag.String("kubeconfig", os.Getenv("KUBECONFIG"), "Path to a kubeconfig. Only required if out-of-cluster.")


Can we actually remove these altogether? They should come via sharedmain anyway, I think? (modulo recursive)

How can we access these arguments from sharedmain?

config, err := mf.NewManifest(filepath.Join(koDataDir, "knative-serving/"), *recursive, cfg)

To create Manifest, we need the cfg. The cfg is created by

cfg, err := clientcmd.BuildConfigFromFlags(*MasterURL, *Kubeconfig)

so we need MasterURL and Kubeconfig. How can we access them in sharedmain?

You're right indeed, we can't reuse them cause they are captured in MainWithConfig.

I'd propose to make these flags package local (lowercase), use the plain Main function in our cmd/manager/main.go and reuse sharedmain.GetConfig(*masterURL, *kubeconfig) here locally. That function does all the defaulting needed by all controllers.

Is it sharedmain.GetConfig("", "")?

well, if you want to keep supporting the flags you gotta pass them in.

@markusthoemmes I made some changes.

markusthoemmes

/lgtm
/approve

/hold

for the nit. Feel free to unhold if you don't want to fix that.

markusthoemmes · 2020-02-13T07:03:05Z

pkg/reconciler/knativeserving/controller.go

-	MasterURL  = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
-	Kubeconfig = flag.String("kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
+	masterURL  = flag.String("master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
+	kubeconfig = flag.String("kubeconfig", os.Getenv("KUBECONFIG"), "Path to a kubeconfig. Only required if out-of-cluster.")


The Getenv call is no longer needed as the GetConfig call below does that defaulting already.

Right! Thanks for the heads up.

aliok · 2020-02-13T07:07:28Z

Squashed and ready for final review

aliok · 2020-02-13T07:10:02Z

for the nit. Feel free to unhold if you don't want to fix that.

I can't. I lack permissions :)

markusthoemmes

/lgtm
/approve
/hold cancel

aliok · 2020-02-13T07:43:16Z

oh man this failed :(

flag redefined: master

Can't redefine a flag. We either need to make the flags in sharedmain exposed or do some hack around reading the same flag again in our controller.

@markusthoemmes wdyt?

markusthoemmes · 2020-02-13T07:45:15Z

Dang. For this PR I'd roll back the change to the "normal" Main function and use the sharedmain.GetConfig helper in main.go as well. That'll land your initial reason to open the PR in the first place.

We can then discuss what to do with the rest.

aliok · 2020-02-13T09:44:43Z

/retest

knative-prow-robot · 2020-02-13T09:45:10Z

@aliok: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

aliok · 2020-02-13T09:47:05Z

Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

oh boy.
cc @markusthoemmes

markusthoemmes · 2020-02-13T10:06:43Z

/ok-to-test

markusthoemmes

/lgtm
/approve

houshengbo

/lgtm
/approve

knative-prow-robot · 2020-02-13T15:32:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, houshengbo, markusthoemmes

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

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

* Support reading KubeConfig from KUBECONFIG env var (#293) * Add upgrade tests into the e2e-tests-latest-serving (#286) * Add upgrade tests into the e2e-tests-latest-serving This PR adds the tests to verify the correct number and names of knative serving deployments. The test tag postupgrade is added, marking the tests to run after upgrade to the latest HEAD of operator, with the latest generated manifest of knative serving. * Install the latest release of the operator and then upgrade * Split the long functions and rename the common package * Refactor the PR based on the comments * Update the library from jcrossley3/manifestival to manifestival * Update pkg/reconciler/knativeserving/common/gateway.go please fix Co-Authored-By: Matt Moore <[email protected]> Co-authored-by: Ali Ok <[email protected]> Co-authored-by: Kenny Leung <[email protected]> Co-authored-by: Matt Moore <[email protected]>

googlebot added the cla: yes Author(s) signed a CLA. label Feb 12, 2020

aliok requested a review from houshengbo February 12, 2020 10:57

knative-prow-robot reviewed Feb 12, 2020

View reviewed changes

knative-prow-robot added needs-ok-to-test size/XS labels Feb 12, 2020

knative-prow-robot requested review from bbrowning and trshafer February 12, 2020 10:57

markusthoemmes reviewed Feb 12, 2020

View reviewed changes

knative-prow-robot added size/S and removed size/XS labels Feb 13, 2020

markusthoemmes approved these changes Feb 13, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold label Feb 13, 2020

knative-prow-robot assigned markusthoemmes Feb 13, 2020

knative-prow-robot added the lgtm label Feb 13, 2020

markusthoemmes mentioned this pull request Feb 13, 2020

Remove all the arguments for controller #294

Closed

aliok force-pushed the read-kubeconfig-env-var branch from 777c53a to 6287419 Compare February 13, 2020 07:07

knative-prow-robot removed the lgtm label Feb 13, 2020

markusthoemmes approved these changes Feb 13, 2020

View reviewed changes

knative-prow-robot added lgtm and removed do-not-merge/hold labels Feb 13, 2020

aliok force-pushed the read-kubeconfig-env-var branch from 6287419 to 6beb48f Compare February 13, 2020 07:58

knative-prow-robot added size/XS and removed lgtm size/S labels Feb 13, 2020

knative-prow-robot added ok-to-test and removed needs-ok-to-test labels Feb 13, 2020

Support reading KubeConfig from KUBECONFIG env var

a81cee7

aliok force-pushed the read-kubeconfig-env-var branch from 6beb48f to a81cee7 Compare February 13, 2020 10:54

aliok mentioned this pull request Feb 13, 2020

Build config consistently as per in serving knative/eventing-operator#105

Merged

markusthoemmes approved these changes Feb 13, 2020

View reviewed changes

knative-prow-robot added the lgtm label Feb 13, 2020

houshengbo approved these changes Feb 13, 2020

View reviewed changes

knative-prow-robot assigned houshengbo Feb 13, 2020

knative-prow-robot added the approved label Feb 13, 2020

knative-prow-robot merged commit a4d4f70 into knative:master Feb 13, 2020

aliok deleted the read-kubeconfig-env-var branch February 14, 2020 07:14

houshengbo pushed a commit to houshengbo/serving-operator that referenced this pull request Feb 17, 2020

Support reading KubeConfig from KUBECONFIG env var (knative#293)

592b320

houshengbo pushed a commit to houshengbo/serving-operator that referenced this pull request Feb 17, 2020

Support reading KubeConfig from KUBECONFIG env var (knative#293)

34e0105

Support reading KubeConfig from KUBECONFIG env var #293

Support reading KubeConfig from KUBECONFIG env var #293

Uh oh!

Conversation

aliok commented Feb 12, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliok commented Feb 13, 2020

Uh oh!

aliok commented Feb 13, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

aliok commented Feb 13, 2020

Uh oh!

markusthoemmes commented Feb 13, 2020

Uh oh!

aliok commented Feb 13, 2020

Uh oh!

knative-prow-robot commented Feb 13, 2020

Uh oh!

aliok commented Feb 13, 2020

Uh oh!

markusthoemmes commented Feb 13, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

houshengbo left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants