Set current namespace from kubeconfig by default by nak3 · Pull Request #172 · knative/client

nak3 · 2019-06-09T10:30:47Z

kn command does not pick up namespace from kubeconfig but hardcorded
default namespace.

This patch fixes to get namespace from kubeconfig.

Fixes #7

knative-prow-robot · 2019-06-09T10:31:03Z

Hi @nak3. 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.

nak3 · 2019-06-09T10:45:30Z

-}
-
-// test with all-namespace flag not defined and no namespace given
-func TestGetNamespaceDefaultAllNamespacesNotDefined(t *testing.T) {


I have removed this test case because it is almost same with TestGetNamespaceDefault. (Please let me know if I should not remove it.)

rhuss · 2019-06-09T12:01:34Z

Coolio, thanks a lot ! 'was just about to fix that on my own, but even better so :-)

I will have a look later today ...

rhuss

Thanks @nak3 ! Looks good for me with some minor comments. Especially getting rid of KubeCfgFile as a global variable which is initalized in one init method and used in another init method is calling for a race sometime in the future.

rhuss · 2019-06-09T19:31:05Z

 type KnParams struct {
 	Output         io.Writer
 	ServingFactory func() (serving.ServingV1alpha1Interface, error)
+	// CurrentNamespace is the namespace set in the current context.


I think it's clear what CurrentNamespace is. So please either remove the comment (recommended) or comment on the other parameters, too.

rhuss · 2019-06-09T19:45:10Z

 		c.ServingFactory = GetConfig
 	}
+	var err error
+	c.CurrentNamespace, err = GetCurrentNamespace(KubeCfgFile)


I wonder whether we could get rid of sharing that global variable and use a function instead which could cache a lookup. This would free us for knowing the order when this variable was initialized (which I just checked, looks good, but you don't know what happens when people refactor code).

I.e. introducing a GetKubeCfgFile() function which checks a private variable kubeCfgFile and, if empty, does the actual lookup as it's done in InitializeConfig:

func GetKubeCfgFile() string { if kubeCfgFile == "" { kubeCfgFile = initKubeConfig() } return kubeCfgFile }

(and maybe improve the error handling over there, as initKubeConfig() must not call os.Exit() but should return an error which should be propagated here, too.

Agree, I made it public and hated that. Better to go back to old Java-style getters for this one and hide the var. Thanks for suggesting this.

rhuss · 2019-06-09T19:46:33Z

 }
+
+// GetCurrentNamespace returns current namespace from kubeconfig.
+func GetCurrentNamespace(kubeconfig string) (string, error) {


can't really comment on the logic here as I don't know anything about how to extract a current context from the configuration. Looks complicated though ;-)

Isn’t it one line in the KUBECONFIG file? Maybe my clusters are super simple...

rhuss · 2019-06-10T06:50:49Z

/ok-to-test

nak3 · 2019-06-10T07:37:47Z

Thank you @rhuss Updated (and moved KubeCfgFile into KnParams struct.)

maximilien · 2019-06-10T18:58:33Z

-	p.Initialize()
+	err := p.Initialize()
+	if err != nil {
+		panic(fmt.Sprintf("Failed to initialize command %v", err))


Wondering if panic is the right thing to do here? I can understand the rational but the error to user will be kind of ugly. Returning error with message will be handled like all errors in main.go...

Though I can also see the argument that this is not a common error and panicking is warranted. Conflicted a bit.

+1 for not panicing but exiting with a "normal" error. I think it's not impossible that initialization fails, e.g. when some .kube config file is not readable. We always strive for nice error messages.

nak3 · 2019-06-11T02:08:00Z

Thank you @maximilien and @rhuss. Updated.

I realized that Initalize() should not cause panic nor catch error as current code does. If we handle error, users must set kubeconfig even when we just want to check some help usage like kn -h.

cppforlife · 2019-06-11T21:37:36Z

+		}
+	}
+	rules := clientcmd.NewDefaultClientConfigLoadingRules()
+	rules.ExplicitPath = c.KubeCfgFile


see https://github.com/k14s/kapp/blob/0c91e8a2208ff9a8f9d92f8c01d20dc9dd21256b/pkg/kapp/cmd/core/config_factory.go#L60 for example. we dont need to specify ExplicitPath unless it's given as an explicit option. also KUBECONFIG env var will be picked up by default.

nak3 · 2019-06-12T01:09:44Z

Thank you @cppforlife Updated.

cppforlife · 2019-06-13T20:51:05Z

+			return nil, err
+		}
+	}
+	config, err := clientcmd.BuildConfigFromFlags("", c.KubeCfgFile)


we should also get ClientConfig() from clientcmd.NewNonInteractiveDeferredLoadingClientConfig() instead of doing initKubeConfig(). i dont think we actually need initKubeConfig at all.

nak3 · 2019-06-14T08:06:15Z

/test pull-knative-client-go-coverage

nak3 · 2019-06-14T08:13:56Z

@cppforlife Thank you! updated.

cppforlife · 2019-06-17T19:24:36Z

+	expectedNamespace := "current"
 	testCmd.Execute()
-	actualNamespace, err := GetNamespace(testCmd)
+	kp := &KnParams{CurrentNamespace: "current"}


i think you should add this line to every test case since it's implied that CurrentNamespace() from clientcmd is set. additionally is there an easy way to test GetConfig(). if not in unit test land, can it be done in e2e?

cppforlife · 2019-06-17T19:25:35Z

+	}
+	c.ClientConfig = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, &clientcmd.ConfigOverrides{})
+	var err error
+	c.CurrentNamespace, _, err = c.ClientConfig.Namespace()


not idea that GetConfig() mutates KnParams. is there a better pattern we can have?

nak3 · 2019-06-19T17:02:18Z

Thank you @cppforlife ! Updated.

Currently kn command does not pick up namespace from kubeconfig but hardcorded default namespace. This patch fixes to get namespace from kubeconfig. Fixes #7

navidshaikh · 2019-06-20T12:15:16Z

@nak3 : oops

➜  client git:(current-ns) ./kn version
Version:      v20190620-local-0ce1486
Build Date:   2019-06-20 12:10:25
Git Revision: 0ce1486
Dependencies:
- serving:    v0.6.0

➜  client git:(current-ns) ./kn service create current-ns --image gcr.io/knative-samples/helloworld-go                                                                               
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0xf3124a]

goroutine 1 [running]:
github.com/knative/client/pkg/kn/commands.(*KnParams).CurrentNamespace(0xc0002c1740, 0xc0002d0d50, 0x120eadc, 0xe, 0x1d00b60)
	/home/nshaikh/go/src/github.com/knative/client/pkg/kn/commands/types.go:47 +0x2a
github.com/knative/client/pkg/kn/commands.(*KnParams).CurrentNamespace-fm(0xc0000f6f00, 0x120eadc, 0xe, 0x0)
	/home/nshaikh/go/src/github.com/knative/client/pkg/kn/commands/types.go:42 +0x2a
github.com/knative/client/pkg/kn/commands.(*KnParams).GetNamespace(0xc0002c1740, 0xc0002e9180, 0x32, 0x1d00b60, 0x0, 0x0)
	/home/nshaikh/go/src/github.com/knative/client/pkg/kn/commands/namespaced.go:58 +0xe1
github.com/knative/client/pkg/kn/commands/service.NewServiceCreateCommand.func1(0xc0002e9180, 0xc0002d1140, 0x1, 0x3, 0x0, 0x0)
	/home/nshaikh/go/src/github.com/knative/client/pkg/kn/commands/service/service_create.go:62 +0xde
github.com/spf13/cobra.(*Command).execute(0xc0002e9180, 0xc0002d10e0, 0x3, 0x3, 0xc0002e9180, 0xc0002d10e0)
	/home/nshaikh/go/src/github.com/knative/client/vendor/github.com/spf13/cobra/command.go:762 +0x473
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002e8780, 0xc0002f6280, 0xc0002e9900, 0xc0002e8a00)
	/home/nshaikh/go/src/github.com/knative/client/vendor/github.com/spf13/cobra/command.go:852 +0x2fd
github.com/spf13/cobra.(*Command).Execute(0xc0002e8780, 0x0, 0x0)
	/home/nshaikh/go/src/github.com/knative/client/vendor/github.com/spf13/cobra/command.go:800 +0x2b
main.main()
	/home/nshaikh/go/src/github.com/knative/client/cmd/kn/main.go:25 +0x44

➜  client git:(current-ns) git checkout master && hack/build.sh -f && ./kn service create current-ns --image gcr.io/knative-samples/helloworld-go
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
🚧 Compile
────────────────────────────────────────────
Version:      v20190620-local-32ccc33
Build Date:   2019-06-20 12:14:09
Git Revision: 32ccc33
Dependencies:
- serving:    v0.6.0
Service 'current-ns' successfully created in namespace 'default'.

knative-metrics-robot · 2019-06-20T12:26:59Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/namespaced.go	92.3%	87.5%	-4.8

navidshaikh

/lgtm

knative-prow-robot · 2019-06-20T13:05:43Z

@navidshaikh: changing LGTM is restricted to assignees, and only knative/client repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

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.

navidshaikh · 2019-06-20T14:07:12Z

Well! pull-knative-client-integration-tests dashboard shows SUCCESS but here its Job failed!

navidshaikh · 2019-06-20T14:07:38Z

/retest

csantanapr · 2019-06-24T13:15:00Z

/lgtm

cppforlife · 2019-06-24T18:12:29Z

/approve
/lgtm

knative-prow-robot · 2019-06-24T18:12:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cppforlife, nak3, navidshaikh

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~~ [cppforlife]

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

* Set current namespace from kubeconfig by default Currently kn command does not pick up namespace from kubeconfig but hardcorded default namespace. This patch fixes to get namespace from kubeconfig. Fixes knative#7 * Use NamespaceFactory to get current namespace * Update unit tests * Add nil check for ClientConfig

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 9, 2019

knative-prow-robot requested review from mattmoor and sixolet June 9, 2019 10:30

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2019

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 9, 2019

nak3 commented Jun 9, 2019

View reviewed changes

rhuss suggested changes Jun 9, 2019

View reviewed changes

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019

maximilien reviewed Jun 10, 2019

View reviewed changes

cppforlife reviewed Jun 11, 2019

View reviewed changes

rhuss mentioned this pull request Jun 13, 2019

Need a KN_NAMESPACE env variable to avoid repeating same namespace #184

Closed

cppforlife reviewed Jun 13, 2019

View reviewed changes

cppforlife reviewed Jun 17, 2019

View reviewed changes

Comment thread pkg/kn/commands/types.go Outdated

cppforlife reviewed Jun 17, 2019

View reviewed changes

nak3 added 3 commits June 20, 2019 19:39

Set current namespace from kubeconfig by default

40ff660

Currently kn command does not pick up namespace from kubeconfig but hardcorded default namespace. This patch fixes to get namespace from kubeconfig. Fixes #7

Use NamespaceFactory to get current namespace

2c90619

Update unit tests

0ce1486

Add nil check for ClientConfig

5838a15

navidshaikh approved these changes Jun 20, 2019

View reviewed changes

mattmoor removed their request for review June 21, 2019 03:16

navidshaikh mentioned this pull request Jun 24, 2019

kn fails when KUBECONFIG multi config files #196

Closed

knative-prow-robot assigned csantanapr Jun 24, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019

knative-prow-robot assigned cppforlife Jun 24, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2019

knative-prow-robot merged commit 13ff277 into knative:master Jun 24, 2019

nak3 deleted the current-ns branch June 24, 2019 22:52

cppforlife mentioned this pull request Jul 2, 2019

Support multi config files #222

Merged

Conversation

nak3 commented Jun 9, 2019

Uh oh!

knative-prow-robot commented Jun 9, 2019

Uh oh!

nak3 Jun 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jun 9, 2019

Uh oh!

rhuss 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!

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!

rhuss commented Jun 10, 2019

Uh oh!

nak3 commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 14, 2019

Uh oh!

nak3 commented Jun 14, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 19, 2019

Uh oh!

navidshaikh commented Jun 20, 2019

Uh oh!

knative-metrics-robot commented Jun 20, 2019

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jun 20, 2019

Uh oh!

navidshaikh commented Jun 20, 2019

Uh oh!

navidshaikh commented Jun 20, 2019

Uh oh!

csantanapr commented Jun 24, 2019

Uh oh!

cppforlife commented Jun 24, 2019

Uh oh!

knative-prow-robot commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nak3 Jun 9, 2019 •

edited

Loading