Skip to content
This repository has been archived by the owner on Nov 19, 2020. It is now read-only.

list/watch resources across all namespaces #32

Merged
merged 1 commit into from
Feb 28, 2017

Conversation

ultimateboy
Copy link
Contributor

Fixes #21

@ericchiang
Copy link
Owner

@ultimateboy I was thinking about this recently and I don't know if I like the current behavior for Lists. E.g. that this

apps.ListPetSets(ctx, "")

Defaults to the client's "default" namespace. It's not very explicit.

Maybe we could make the "" value default to AllNamespaces like in the official client?

const (
    // AllNamespaces is given to list and watch operations to signify that the code should
    // list or watch resources in all namespaces.
    AllNamespaces = allNamespaces
    // Actual definition is private in case we want to change it later.
    allNamespace = ""
)

So you can do

apps.ListPetSets(ctx, k8s.AllNamespaces)

That would prevent adding more methods and clean up the ambiguous of

apps.ListPetSets(ctx, "")

What do you think?

@ultimateboy
Copy link
Contributor Author

I'd be ok with this. I was actually kinda surprised that passing an empty string meant the pod's namespace instead of all-namespaces. I implemented it this way because it's what was in the original issue and I did not want to break backwards compatibility.

@ultimateboy
Copy link
Contributor Author

@ericchiang Let me know if you want me to go ahead and update this PR with the proposed new direction. I would ask that you make a new release before we implement breaking changes.

@ericchiang
Copy link
Owner

@ultimateboy yep, go ahead and update. I'll create a release once you're done and call out the change.

@ultimateboy
Copy link
Contributor Author

@ericchiang ready for review

@ericchiang
Copy link
Owner

@ultimateboy awesome! could you add a test? (also can you run the tests? I've only ever tried on my machine) https://github.com/ericchiang/k8s/blob/master/client_test.go

Changes previous default behavior where an empty string namespace would
use the client's namespace instead of all-namespaces. Now, an emtpy
string equates to all-namespaces (there is a k8s.AllNamespaces constant
as well).
@ultimateboy
Copy link
Contributor Author

@ericchiang Test has been added.

$ make test
go test -v ./...
=== RUN   TestNewTestClient
--- PASS: TestNewTestClient (0.03s)
=== RUN   TestHTTP2
--- PASS: TestHTTP2 (0.05s)
=== RUN   TestListNodes
--- PASS: TestListNodes (0.04s)
=== RUN   TestConfigMaps
--- PASS: TestConfigMaps (0.05s)
=== RUN   TestWatch
--- PASS: TestWatch (0.05s)
=== RUN   TestWatchNamespace
--- PASS: TestWatchNamespace (0.08s)
=== RUN   TestDiscovery
--- PASS: TestDiscovery (0.04s)
=== RUN   TestLabelSelector
--- PASS: TestLabelSelector (0.00s)
=== RUN   TestTPRs
--- PASS: TestTPRs (0.07s)
PASS
ok  	github.com/ericchiang/k8s	0.423s
?   	github.com/ericchiang/k8s/api/resource	[no test files]
?   	github.com/ericchiang/k8s/api/unversioned	[no test files]
?   	github.com/ericchiang/k8s/api/v1	[no test files]
?   	github.com/ericchiang/k8s/apis/apps/v1alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/apps/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/apis/authentication/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/apis/authorization/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/apis/autoscaling/v1	[no test files]
?   	github.com/ericchiang/k8s/apis/batch/v1	[no test files]
?   	github.com/ericchiang/k8s/apis/batch/v2alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/certificates/v1alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/extensions/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/apis/imagepolicy/v1alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/policy/v1alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/policy/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/apis/rbac/v1alpha1	[no test files]
?   	github.com/ericchiang/k8s/apis/storage/v1beta1	[no test files]
?   	github.com/ericchiang/k8s/examples	[no test files]
?   	github.com/ericchiang/k8s/runtime	[no test files]
?   	github.com/ericchiang/k8s/util/intstr	[no test files]
?   	github.com/ericchiang/k8s/watch/versioned	[no test files]

@ericchiang
Copy link
Owner

lgtm!

@ericchiang ericchiang merged commit 9fe8b1e into ericchiang:master Feb 28, 2017
@ericchiang
Copy link
Owner

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants