svcat CLI can now filter the classes return by broker in `svcat get classes` via a `--broker` flag #2786

gmrodgers · 2020-03-20T17:02:55Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:
Adds filtering by broker to the svcat get classes subcommand via the --broker flag.
It also consequencely adds the broker providing the class to the table output of the get command.

Which issue(s) this PR fixes
Partially fixes #1749 but does not include the plans filtering (personally I filtering by broker as more appropriate on the classes as it almost the "parent" of that. Let me know if you feel differently!

One think to note is that I've added a BrokerFiltered property to the GetCmd, I see that similar things are done on some commands, such as get instances but others like get plans uses a ClassFilter string. I chose the former but I would be interested if there was any context between the two choices!

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

k8s-ci-robot · 2020-03-20T17:03:06Z

Hi @gmrodgers. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

k8s-ci-robot · 2020-03-20T17:03:07Z

Welcome @gmrodgers!

It looks like this is your first PR to kubernetes-sigs/service-catalog 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/service-catalog has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

jberkhahn · 2020-03-20T19:54:13Z

looks like make verify didn't like your comment formatting?

gmrodgers · 2020-03-20T20:13:33Z

Yep! Having problems getting verify running on my machine! Will fix that commit and try and get that sorted

mszostok · 2020-03-20T20:15:20Z

/ok-to-test

jberkhahn · 2020-03-20T22:09:31Z

other than the comment hiccup this looks great!
/lgtm

gmrodgers · 2020-03-23T12:26:39Z

Ive added in the License that my previous commit removed, seems one of the make commands popped it out but still seems to be breaking. My colleague is working on a branch hopefully that should mitigate some of the make verify problems. Sorry for the blindish commits, hard to replicate locally!

mszostok · 2020-03-21T17:27:16Z

pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go

@@ -1,35 +1,19 @@
-/*


Unfortunately, this comment needs to be reverted:

Boilerplate header is wrong for: ././pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go

IMO that this should be skipped by checker but somehow the generated files are not ignored.

mszostok · 2020-03-23T12:32:08Z

Hi. @gmrodgers thanks for your PR! :)

you can always check the travis job: https://travis-ci.org/github/kubernetes-sigs/service-catalog/jobs/665834302?utm_medium=notification&utm_source=github_status

at the end of logs you will see there problem reported by verify

Boilerplate header is wrong for: ././pkg/svcat/service-catalog/service-catalogfakes/fake_svcat_client.go
Makefile:207: recipe for target 'verify' failed
make: *** [verify] Error 1

I also describe it here: #2786 (comment)

the make verify can be also executed locally. I'm able to do it on MacOS

I will take a look on that PR today (approximately in ~2h) :)

gmrodgers · 2020-03-23T16:50:40Z

Hey @mszostok I've reverted the changed to the boilerplate, hopefully will flow through now :)

cmd/svcat/command/brokerfiltered.go

mszostok · 2020-03-23T17:24:10Z

cmd/svcat/browsing/marketplace_cmd.go

@@ -66,7 +66,7 @@ func (c *MarketplaceCmd) Run() error {
 		Namespace: c.Namespace,
 		Scope:     servicecatalog.AllScope,
 	}
-	classes, err := c.App.RetrieveClasses(opts)
+	classes, err := c.App.RetrieveClasses(opts, "")


I need to say that I do not like such API which requires from you to set some values when you are not interested in customizing that. IMO the best option here will be the functional options pattern. As a result you will have

# default behaviour c.App.RetrieveClasses() # sth changed c.App.RetrieveClasses(WithBrokerName("myqsl"), InNamespace("system"))

benefits:

in future you do not need to break the function signature (always backward compatible) (and this is already named as SDK so it is used by others)

only can change the default behavior only if want without setting some additional values

but I joined the SC after the svcat was introduced so maybe let's do not do such huge refactor as you already did a nice work 👍

but at least let's try to avoid the magic value and introduce const for that case, similar approach is used in k8s for empty string in clients:
https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/types.go#L287-L290

so sth like c.App.RetrievClasses(opts, FromAllBrokers)

mszostok · 2020-03-23T17:33:29Z

I've tested that and it works like a charm :)

Please see this comment: #2786 (comment) and introduce that const and this will be ready to merge 👍

jberkhahn · 2020-03-23T21:10:05Z

I think making it a const so the blank value seems less magic is a good idea. Not sure how I feel about the funcs as args, though.

A few things are causing the `make verify` command (and subsequently `make`) to fail. These are addressed in this commit, as follows: 1. `make verify-docs` - Explicitely skip the localhost link in contribute/docs.md and remove a link for a non-yet-existent repository in pkg/kubernetes/README.md. 1. Exclude `bin/verify-links.sh` from the `verify-boilerplate.sh` check (this file is .gitignored) 1. Update GO_VERSION defined in the Makefile to match GO_VERSION defined in .travis.yml Addresses an issue mentioned in kubernetes-retired#2782.

…retired#2790)

gmrodgers · 2020-03-27T15:20:42Z

Hmmm so I squashed the commit into one feature commit ~~but Im struggling to see whats causing the error above as it's logically equivalent to the one with the magic empty strings? I can take more of a look at it next friday when I have time~~ seems to have passed now

mszostok · 2020-03-27T19:34:51Z

/lgtm

jberkhahn · 2020-03-27T19:55:50Z

/approve

k8s-ci-robot · 2020-03-27T19:56:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmrodgers, jberkhahn

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

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 20, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 20, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 20, 2020

k8s-ci-robot requested review from jberkhahn and piotrmiskiewicz March 20, 2020 17:03

k8s-ci-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 Mar 20, 2020

k8s-ci-robot assigned jberkhahn Mar 20, 2020

k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 20, 2020

mszostok reviewed Mar 23, 2020

View reviewed changes

gmrodgers force-pushed the filter-by-broker-1749 branch from 639299e to 2950e28 Compare March 23, 2020 16:48

mszostok reviewed Mar 23, 2020

View reviewed changes

cmd/svcat/command/brokerfiltered.go Outdated Show resolved Hide resolved

mszostok approved these changes Mar 23, 2020

View reviewed changes

teddyking and others added 3 commits March 27, 2020 14:42

Update ups-broker docs link to point to chart on github. (kubernetes-…

2f10cfe

…retired#2790)

Support filtering classes by broker in svcat

38729e2

gmrodgers force-pushed the filter-by-broker-1749 branch from 7281456 to 38729e2 Compare March 27, 2020 15:03

k8s-ci-robot assigned mszostok Mar 27, 2020

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2020

k8s-ci-robot merged commit ed186ba into kubernetes-retired:master Mar 27, 2020

mszostok mentioned this pull request May 25, 2020

Update version from 0.3.0-beta.2 to 0.3.0 #2811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

svcat CLI can now filter the classes return by broker in `svcat get classes` via a `--broker` flag #2786

svcat CLI can now filter the classes return by broker in `svcat get classes` via a `--broker` flag #2786

gmrodgers commented Mar 20, 2020 •

edited

Loading

k8s-ci-robot commented Mar 20, 2020

k8s-ci-robot commented Mar 20, 2020

jberkhahn commented Mar 20, 2020

gmrodgers commented Mar 20, 2020

mszostok commented Mar 20, 2020

jberkhahn commented Mar 20, 2020

gmrodgers commented Mar 23, 2020

mszostok Mar 21, 2020

mszostok commented Mar 23, 2020 •

edited

Loading

gmrodgers commented Mar 23, 2020

mszostok Mar 23, 2020

mszostok commented Mar 23, 2020

jberkhahn commented Mar 23, 2020

gmrodgers commented Mar 27, 2020 •

edited

Loading

mszostok commented Mar 27, 2020

jberkhahn commented Mar 27, 2020

k8s-ci-robot commented Mar 27, 2020

svcat CLI can now filter the classes return by broker in svcat get classes via a --broker flag #2786

svcat CLI can now filter the classes return by broker in svcat get classes via a --broker flag #2786

Conversation

gmrodgers commented Mar 20, 2020 • edited Loading

k8s-ci-robot commented Mar 20, 2020

k8s-ci-robot commented Mar 20, 2020

jberkhahn commented Mar 20, 2020

gmrodgers commented Mar 20, 2020

mszostok commented Mar 20, 2020

jberkhahn commented Mar 20, 2020

gmrodgers commented Mar 23, 2020

mszostok Mar 21, 2020

Choose a reason for hiding this comment

mszostok commented Mar 23, 2020 • edited Loading

gmrodgers commented Mar 23, 2020

mszostok Mar 23, 2020

Choose a reason for hiding this comment

mszostok commented Mar 23, 2020

jberkhahn commented Mar 23, 2020

gmrodgers commented Mar 27, 2020 • edited Loading

mszostok commented Mar 27, 2020

jberkhahn commented Mar 27, 2020

k8s-ci-robot commented Mar 27, 2020

svcat CLI can now filter the classes return by broker in `svcat get classes` via a `--broker` flag #2786

svcat CLI can now filter the classes return by broker in `svcat get classes` via a `--broker` flag #2786

gmrodgers commented Mar 20, 2020 •

edited

Loading

mszostok commented Mar 23, 2020 •

edited

Loading

gmrodgers commented Mar 27, 2020 •

edited

Loading