make svcat describe and get broker now support namespaced resources #2564

jberkhahn · 2019-02-07T23:39:47Z

This PR is a

[x ] Feature Implementation
[ ]= Bug Fix
Documentation

What this PR does / why we need it:
per #2451 as part of the burndown on namespace resources, this changes the behavior of svcat get and describe to as follows:

user asks for some broker named foo
look for clusterservicebrokers and servicebrokers in the current namespace named foo
-if only 1 is found, return it
if >1 are found, ask the user to specify scope

This also adds a typeline to the various places brokers are displayed to the user to inform them if its a clusterservicebroker or servicebroker

Which issue(s) this PR fixes
#2451

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

jberkhahn · 2019-02-12T23:12:15Z

/retest

jberkhahn · 2019-02-14T22:38:54Z

/cc @MHBauer could you take a look at this when you have some time?

MHBauer · 2019-02-15T00:43:11Z

pretty big, could take a while

MHBauer · 2019-02-19T00:15:32Z

cc needs to be on it's own line
/cc @MHBauer

MHBauer · 2019-02-19T00:18:09Z

or it's just broke

MHBauer · 2019-02-19T00:20:26Z

nope, there it is.

…

On 2/18/19 16:15:33, Kubernetes Prow Robot wrote: @k8s-ci-robot <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_k8s-2Dci-2Drobot&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=jHNqYcwlFE6ehT68HxH9rhGt2krybG3s5p-VIv4Kg4c&s=iRMbsHw_U6nXOvDSaZk6PPIsmpJ_SXNHpVBBDx3Nbds&e=> requested your review on: #2564 <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_kubernetes-2Dincubator_service-2Dcatalog_pull_2564&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=jHNqYcwlFE6ehT68HxH9rhGt2krybG3s5p-VIv4Kg4c&s=y8ZuVThaNx7nCnd_Zuz1PcHlDN24plU6jtMqMq9wXDw&e=> make svcat describe and get broker now support namespaced resources. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_kubernetes-2Dincubator_service-2Dcatalog_pull_2564-23event-2D2147496661&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=jHNqYcwlFE6ehT68HxH9rhGt2krybG3s5p-VIv4Kg4c&s=3lWANSwgYl6_U4XpXqY0haYkU-i4FaXrZ2APjpstbek&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMuDAbZM8x-5Fi-2DVopjtITazcUuYkcHY4yks5vO0IlgaJpZM4aq1Xa&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=jHNqYcwlFE6ehT68HxH9rhGt2krybG3s5p-VIv4Kg4c&s=mjFE64k5UHNjDOr4lngaPoa30bBN3H0IYr67O2K9zMo&e=>.

MHBauer

sorry for taking so long. I see tiny nits and have questions.

Only one instance of "Why did you write it that way".

MHBauer · 2019-03-05T17:08:14Z

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

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Kubernetes Authors.


mock changes, ignoring.

MHBauer · 2019-03-05T17:09:56Z

pkg/svcat/service-catalog/sdk.go

@@ -46,7 +46,7 @@ type SvcatClient interface {

 	Deregister(string, *ScopeOptions) error
 	RetrieveBrokers(opts ScopeOptions) ([]Broker, error)
-	RetrieveBroker(string) (*apiv1beta1.ClusterServiceBroker, error)
+	RetrieveBrokerByID(string, ScopeOptions) (Broker, error)


why change the method name?

missed the scope addition.

MHBauer · 2019-03-05T17:11:11Z

cmd/svcat/output/broker.go

@@ -23,6 +23,13 @@ import (
 	"github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog"
 )

+func getBrokerScope(broker servicecatalog.Broker) string {
+	if broker.GetNamespace() != "" {


is empty string already resolved to "default" at this point?

Note that this isn't an actual v1beta1 broker, it's our custom-spun interface that spans the two types for svcat to use - this always returns "" for CSB's: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/apis/servicecatalog/v1beta1/broker.go#L30

MHBauer · 2019-03-05T17:12:33Z

cmd/svcat/testdata/plugin.yaml

@@ -126,6 +126,9 @@ tree:
    use: binding NAME


cmd/svcat/svcat_test.go

MHBauer · 2019-03-05T17:16:54Z

cmd/svcat/broker/describe_cmd_test.go

@@ -14,89 +14,217 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */


I trust the @jberkhahn tests.

cmd/svcat/broker/get_cmd_test.go

cmd/svcat/output/broker.go

cmd/svcat/broker/get_cmd.go

cmd/svcat/broker/describe_cmd.go

jberkhahn · 2019-03-06T00:14:34Z

Thanks for the review @MHBauer. Gotta jet atm, but I will get this stuff looked at ASAP.

jberkhahn · 2019-03-07T00:09:13Z

/retest

jberkhahn · 2019-03-07T00:44:37Z

/retest

MHBauer

I need more understanding of how opts matches works.

MHBauer · 2019-03-07T19:41:23Z

cmd/svcat/output/broker.go

+	}
+	table = append(table, []string{"URL:", broker.GetURL()})
+	table = append(table, []string{"Status:", getBrokerStatusFull(broker.GetStatus())})
+	t.AppendBulk(table)


MHBauer · 2019-03-07T19:42:25Z

pkg/svcat/service-catalog/broker.go

+		}
+	}
+
+	if opts.Scope.Matches(NamespaceScope) {


it can't be an else if, its possible to match both

MHBauer · 2019-03-07T19:42:42Z

pkg/svcat/service-catalog/broker.go

+			if err != nil && !apierrors.IsNotFound(err) {
+				return nil, fmt.Errorf("unable to search servicebrokers k8s by name (%s)", err)
+			}
+		}


return sb here ?

MHBauer · 2019-03-07T19:42:50Z

pkg/svcat/service-catalog/broker.go

+		if err != nil && !apierrors.IsNotFound(err) {
+			return nil, fmt.Errorf("unable to search clusterservicebrokers by k8s name (%s)", err)
+		}
+	}


return csb here?

its possible we get both a sb and csb back, if they were created with the same name, which is possible as they're different types

MHBauer · 2019-03-07T20:12:12Z

strange case, makes sense now. gotta handle "all"
/lgtm

MHBauer · 2019-03-07T22:12:25Z

/assign @jboyd01

jboyd01

Sorry it took me so long to review. One nit and a simplification

jboyd01 · 2019-03-08T13:56:25Z

pkg/svcat/service-catalog/broker.go

@@ -29,6 +29,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 )

+// MultipleBrokersFoundError is the error returned when we find a clusterservicebroker
+// and a servicebroker with the same name
+const MultipleBrokersFoundError = "Too many brokers found"


nit: what do you think about "more then one" vs "too many" ?

sounds good.

jboyd01 · 2019-03-08T13:59:25Z

pkg/svcat/service-catalog/broker.go

+				sb = nil
+			}
+			if err != nil && !apierrors.IsNotFound(err) {
+				return nil, fmt.Errorf("unable to search servicebrokers k8s by name (%s)", err)


should this be "unable to search servicebrokers by k8s name" ? (ie the placement of k8s). And should we not spell out kubernetes?

change 135 & 136 to } else { as we already know err != nil

jberkhahn · 2019-03-08T19:35:58Z

@jboyd01 @MHBauer changes are up

-add RetrieveBrokerByID to pkg/svcat

jboyd01 · 2019-03-13T20:49:33Z

/lgtm

MHBauer · 2019-03-13T21:08:34Z

/approve

k8s-ci-robot · 2019-03-13T21:08:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MHBauer

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:

~~cmd/svcat/OWNERS~~ [MHBauer]
~~pkg/svcat/OWNERS~~ [MHBauer]

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

jberkhahn · 2019-03-13T21:32:29Z

/test pull-service-catalog-xbuild

…netes-retired#2564) -add RetrieveBrokerByID to pkg/svcat

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2019

k8s-ci-robot requested review from eriknelson and kikisdeliveryservice February 7, 2019 23:40

jberkhahn force-pushed the namespace_describe branch 8 times, most recently from 1449ee9 to 20267d3 Compare February 12, 2019 22:34

jberkhahn force-pushed the namespace_describe branch from 20267d3 to 66c50ea Compare February 12, 2019 23:21

k8s-ci-robot requested a review from MHBauer February 19, 2019 00:15

MHBauer approved these changes Mar 5, 2019

View reviewed changes

jberkhahn force-pushed the namespace_describe branch from 66c50ea to b59db35 Compare March 6, 2019 23:29

MHBauer approved these changes Mar 7, 2019

View reviewed changes

k8s-ci-robot assigned MHBauer Mar 7, 2019

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

k8s-ci-robot assigned jboyd01 Mar 7, 2019

jboyd01 reviewed Mar 8, 2019

View reviewed changes

jberkhahn force-pushed the namespace_describe branch from b59db35 to 52de896 Compare March 8, 2019 19:35

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2019

svcat describe and get broker now support namespaced resources

ac5837b

-add RetrieveBrokerByID to pkg/svcat

jberkhahn force-pushed the namespace_describe branch from 52de896 to ac5837b Compare March 13, 2019 20:48

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

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

k8s-ci-robot merged commit f8870a5 into kubernetes-retired:master Mar 13, 2019

viviyww pushed a commit to viviyww/service-catalog that referenced this pull request May 10, 2019

svcat describe and get broker now support namespaced resources (kuber…

d0650d2

…netes-retired#2564) -add RetrieveBrokerByID to pkg/svcat

cblecker unassigned jboyd01 and MHBauer Jun 4, 2019

		@@ -14,89 +14,217 @@ See the License for the specific language governing permissions and
		limitations under the License.
		*/

make svcat describe and get broker now support namespaced resources #2564

make svcat describe and get broker now support namespaced resources #2564

Conversation

jberkhahn commented Feb 7, 2019 • edited Loading

jberkhahn commented Feb 12, 2019

jberkhahn commented Feb 14, 2019

MHBauer commented Feb 15, 2019

MHBauer commented Feb 19, 2019

MHBauer commented Feb 19, 2019

MHBauer commented Feb 19, 2019 via email

MHBauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Mar 6, 2019

jberkhahn commented Mar 7, 2019

jberkhahn commented Mar 7, 2019

MHBauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Mar 7, 2019

MHBauer commented Mar 7, 2019

jboyd01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Mar 8, 2019

jboyd01 commented Mar 13, 2019

MHBauer commented Mar 13, 2019

k8s-ci-robot commented Mar 13, 2019

jberkhahn commented Mar 13, 2019

jberkhahn commented Feb 7, 2019 •

edited

Loading