Support multi config files #222

edwardstudy · 2019-07-02T07:59:22Z

Fixed: #196

./kn service list --kubeconfig=$KUBECONFIG
the server could not find the requested resource (get services.serving.knative.dev)

knative-prow-robot · 2019-07-02T07:59:37Z

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

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.

rhuss · 2019-07-02T08:05:13Z

/ok-to-test

rhuss

Thanks for the PR !

Do you think it would be possible to write a test for this, too ? We should do this if the test setup is not so involved that it does not justify the benefit.

pkg/kn/commands/types.go

rhuss

Tanks a lot. Funny thing is that the coverage report fails now after you added a test (and we didn't have a test before that for types.go). So I'm not sure how this check is supposed to work, as it seems like having 0% coverage is ok, but if you have a coverage >0% then you do need to have more than 50%.

Not sure how we can resolve that (except by writing more test code unrelated to this PR).

@sixolet @adrcunha Is there a way to "overrule" the code coverage check ? Or what is the recommendation for this kind of situation ?

adrcunha · 2019-07-02T15:28:07Z

@sixolet @adrcunha Is there a way to "overrule" the code coverage check ? Or what is the recommendation for this kind of situation ?

I think the coverage check was marked as required by mistake, based on the numbers in https://testgrid.knative.dev/client#coverage

Unless you feel very confident that, at this point that no PR with coverage below 50% should get merged, I suggest that an admin simply uncheck the go coverage job as required to merge.

cppforlife · 2019-07-02T18:11:06Z

i dont think kubectl supports following (someone verify:

./kn service list --kubeconfig=$KUBECONFIG

it does support KUBECONFIG having multiple paths set though which we support as well via kubectl's library.

original issue mentioned in this pr is fixed via #172.

rhuss · 2019-07-02T19:57:15Z

good catch, @cppforlife

someone verify:

correct, I get an error with a --kubeconfig=$PATH_LIKE_VAR :

error: stat /Users/roland/.kube/config:/Users/roland/Development/openshift/ocp4/dev/auth/kubeconfig: no such file or directory

@edwardstudy sorry for bugging you with the test and change (really good work, thx again), but what was actually your use case ? Does setting KUBECONFIG as env var to a path does not work as expected ? Or why would you like to have a path like arg for --kubeconfig ?

maximilien · 2019-07-03T01:11:03Z

pkg/kn/commands/types_test.go

+	explicitPath string
+}
+
+func TestGetClientConfig(t *testing.T) {


Nice work @edwardstudy. Consider also using the new gotest.tools which should make your tests cleaner, especially assertions and dividing into multiple parts. We agreed during yesterday's WG meeting to move all tests using this small addition to gotest.tools. See #134's tests for examples or let's chat today in Beijing, I am working on some. Cheers 🍻

edwardstudy · 2019-07-03T03:25:38Z

@rhuss My case is simple, specify multi configs over --kubeconfig flag. When you specify --kubeconfig, you should make sure that kn works no matter what values passed. It should work: get current kube context or return a proper error message. What do you think? 😃

rhuss · 2019-07-03T06:02:00Z

@rhuss My case is simple, specify multi configs over --kubeconfig flag. When you specify --kubeconfig, you should make sure that kn works no matter what values passed. It should work: get current kube context or return a proper error message. What do you think? 😃

For multiple configs, I think that kubectl settle on using an env variable KUBECONFIG for this, so the kubectl is like this (in this order) :

Setting a very specific configuration via --kubeconfig
Setting the location of the configuration files in a path like manner via KUBECONFIG
Fallback to `~/.kube/config

I think your use case can be simply be done with

KUBECONFIG=dir1:dir2 kn service create ....

instead of using one --kubeconfig with a path or multiple kubeconfigs.

We should mimic the behaviour of kubectl here as I think it makes sense, too (no one expects a path with --kubeconfig).

I totally agree thought that the error message should be better, maybe even suggesting that KUBECONFIG env var should be used for multiple configs.

The current kubectl behaviour is:

kubectl get pods --kubeconfig=/Users/roland/.kube/config --kubeconfig=/Users/roland/Development/openshift/ocp4/dev/auth/kubeconfig

---> Take only the last --kubeconfig given

kubectl get pods --kubeconfig=/Users/roland/.kube/config:/Users/roland/Development/openshift/ocp4/dev/auth/kubeconfig

---> error: stat /Users/roland/.kube/config:/Users/roland/Development/openshift/ocp4/dev/auth/kubeconfig: no such file or directory

For the last case, I suggest an error message like

Configuration file "/Users/roland/.kube/config:/Users/roland/Development/openshift/ocp4/dev/auth/kubeconfig" could not be found. For configuration lookup path please use the environment variable KUBECONFIG.

The last sentence I would add when the path contains a : (this might be a valid filename part, too (egg on Windows), but printing out that extra error message wouldn't harm either then. Important is that it should be checked if the provided argument exists as a file first.

@edwardstudy does this make sense ?

edwardstudy · 2019-07-03T15:34:52Z

/retest

edwardstudy · 2019-07-03T15:47:10Z

@rhuss Hi, I recoded and let os.PathListSeparator to handle OS-specific path separator.

rhuss · 2019-07-03T16:09:17Z

Let me jump on the PR later (probably tomorrow, its already late here). Thanks for doing the rework !

rhuss · 2019-07-03T16:10:29Z

/retest

(sorry, test failure is again because of a too short timeout in the it test. We will address this soonish).

rhuss · 2019-07-08T13:10:18Z

/retest

rhuss

direction looks very good. Some minor nits, mostly around testing.

pkg/kn/commands/types.go

pkg/kn/commands/types_test.go

maximilien · 2019-07-10T17:19:48Z

@edwardstudy you also need to rebase to resolve pkg/kn/commands/types.go conflict

rhuss · 2019-07-17T08:44:30Z

pkg/kn/commands/types.go

+
+	paths := filepath.SplitList(params.KubeCfgPath)
+	if len(paths) > 1 {
+		return nil, errors.New(fmt.Sprintf("Can not find config file. '%s' looks like a combine path. "+


Suggested change

return nil, errors.New(fmt.Sprintf("Can not find config file. '%s' looks like a combine path. "+

return nil, errors.New(fmt.Sprintf("Can not find config file. '%s' looks like a path. "+

rhuss

Thanks a lot ! Except for one super-mini correction (where I provide a suggestion) at https://github.com/knative/client/pull/222/files#r304285543 we are good to merge.

edwardstudy · 2019-07-17T08:48:59Z

Done. Sorry for the delay in getting a chance to respond to this PR.

knative-metrics-robot · 2019-07-17T08:52:52Z

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	72.7%	68.0%	-4.7
pkg/kn/commands/types.go	0.0%	53.6%	53.6

rhuss · 2019-07-17T09:02:51Z

/approve
/lgtm

knative-prow-robot · 2019-07-17T09:03:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edwardstudy, rhuss

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

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

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

knative-prow-robot requested review from rhuss and sixolet July 2, 2019 07:59

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 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 Jul 2, 2019

rhuss reviewed Jul 2, 2019

View reviewed changes

pkg/kn/commands/types.go Outdated Show resolved Hide resolved

edwardstudy force-pushed the ed/multi-configs branch from 54ea5af to fb52411 Compare July 2, 2019 10:02

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 2019

edwardstudy force-pushed the ed/multi-configs branch from fb52411 to da126c0 Compare July 2, 2019 10:04

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

rhuss reviewed Jul 2, 2019

View reviewed changes

maximilien reviewed Jul 3, 2019

View reviewed changes

edwardstudy force-pushed the ed/multi-configs branch from da126c0 to 8aececb Compare July 3, 2019 15:31

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 Jul 3, 2019

edwardstudy force-pushed the ed/multi-configs branch from 8aececb to 5d61997 Compare July 3, 2019 15:42

rhuss requested changes Jul 8, 2019

View reviewed changes

edwardstudy added 2 commits July 17, 2019 14:37

Handle error for multi config files over --kubeconfig flag

85211b8

Add test cases for multi configs

1be16cf

edwardstudy force-pushed the ed/multi-configs branch from 5d61997 to 0bdbd06 Compare July 17, 2019 07:13

rhuss reviewed Jul 17, 2019

View reviewed changes

rhuss requested changes Jul 17, 2019

View reviewed changes

edwardstudy force-pushed the ed/multi-configs branch from 0bdbd06 to e9a13d1 Compare July 17, 2019 08:49

Refactor type cases

051e136

edwardstudy force-pushed the ed/multi-configs branch from e9a13d1 to 051e136 Compare July 17, 2019 08:50

knative-prow-robot assigned rhuss Jul 17, 2019

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

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

knative-prow-robot merged commit 0b348d7 into knative:master Jul 17, 2019

edwardstudy deleted the ed/multi-configs branch July 17, 2019 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi config files #222

Support multi config files #222

edwardstudy commented Jul 2, 2019 •

edited

Loading

knative-prow-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019

rhuss left a comment

rhuss left a comment •

edited

Loading

adrcunha commented Jul 2, 2019

cppforlife commented Jul 2, 2019

rhuss commented Jul 2, 2019

maximilien Jul 3, 2019

edwardstudy commented Jul 3, 2019

rhuss commented Jul 3, 2019

edwardstudy commented Jul 3, 2019

edwardstudy commented Jul 3, 2019 •

edited

Loading

rhuss commented Jul 3, 2019

rhuss commented Jul 3, 2019

rhuss commented Jul 8, 2019

rhuss left a comment

maximilien commented Jul 10, 2019

rhuss Jul 17, 2019

rhuss left a comment

edwardstudy commented Jul 17, 2019

knative-metrics-robot commented Jul 17, 2019

rhuss commented Jul 17, 2019

knative-prow-robot commented Jul 17, 2019

	return nil, errors.New(fmt.Sprintf("Can not find config file. '%s' looks like a combine path. "+
	return nil, errors.New(fmt.Sprintf("Can not find config file. '%s' looks like a path. "+

Support multi config files #222

Support multi config files #222

Conversation

edwardstudy commented Jul 2, 2019 • edited Loading

knative-prow-robot commented Jul 2, 2019

rhuss commented Jul 2, 2019

rhuss left a comment

Choose a reason for hiding this comment

rhuss left a comment • edited Loading

Choose a reason for hiding this comment

adrcunha commented Jul 2, 2019

cppforlife commented Jul 2, 2019

rhuss commented Jul 2, 2019

maximilien Jul 3, 2019

Choose a reason for hiding this comment

edwardstudy commented Jul 3, 2019

rhuss commented Jul 3, 2019

edwardstudy commented Jul 3, 2019

edwardstudy commented Jul 3, 2019 • edited Loading

rhuss commented Jul 3, 2019

rhuss commented Jul 3, 2019

rhuss commented Jul 8, 2019

rhuss left a comment

Choose a reason for hiding this comment

maximilien commented Jul 10, 2019

rhuss Jul 17, 2019

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

edwardstudy commented Jul 17, 2019

knative-metrics-robot commented Jul 17, 2019

rhuss commented Jul 17, 2019

knative-prow-robot commented Jul 17, 2019

edwardstudy commented Jul 2, 2019 •

edited

Loading

rhuss left a comment •

edited

Loading

edwardstudy commented Jul 3, 2019 •

edited

Loading