svcat describe and get classes now support namespaced resources #2582

jberkhahn · 2019-03-13T23:08:07Z

This PR is a

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 class named foo
look for clusterserviceclasses and serviceclasses in the current namespace named foo
-if only 1 is found, return it
if >1 are found, ask the user to specify scope

Which issue(s) this PR fixes
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-03-14T23:05:37Z

/cc @jboyd01 @MHBauer

jberkhahn · 2019-04-01T22:44:16Z

cmd/svcat/class/describe_cmd.go

-	kubeName         string
-	name             string
+// DescribeCmd contains the information needed to describe a specific class
+type DescribeCmd struct {


this should have a public context or nothing should have a public context

I don't remember what this means.

jberkhahn · 2019-04-01T23:00:29Z

pkg/svcat/service-catalog/class_test.go

+			Expect(actions[1].Matches("get", "serviceclasses")).To(BeTrue())
+			Expect(actions[1].(testing.GetActionImpl).Name).To(Equal(classID))
+		})
+		It("doesn't short circuit on not-found errors", func() {


add a test for the both-not-found case

MHBauer

looked at with jonathan. his comments are my comments

MHBauer · 2019-03-25T19:24:13Z

pkg/svcat/service-catalog/class.go

-	class, err := sdk.ServiceCatalog().ClusterServiceClasses().Get(kubeName, metav1.GetOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("unable to get class (%s)", err)
+func (sdk *SDK) RetrieveClassByID(kubeName string, opts ScopeOptions) (Class, error) {


same code as previously

MHBauer · 2019-04-01T23:36:31Z

cmd/svcat/class/describe_cmd.go

-		class, err = c.App.RetrieveClassByName(c.name, servicecatalog.ScopeOptions{
-			Scope: servicecatalog.ClusterScope,
-		})
+		class, err = c.App.RetrieveClassByName(c.Name, scopeOpts)


names look funny when compared with previous PR

jberkhahn · 2019-04-03T00:10:00Z

@MHBauer could you take another look at this? fixed your concerns. I'll have to submit another PR to change the naming stuff in the broker commands.

MHBauer

/gtm

MHBauer · 2019-04-03T00:34:47Z

cmd/svcat/class/describe_cmd.go

-	kubeName         string
-	name             string
+// DescribeCmd contains the information needed to describe a specific class
+type DescribeCmd struct {


I don't remember what this means.

jberkhahn · 2019-04-08T22:20:25Z

@MHBauer looks like your LGTM didn't take, could you re /lgtm this?

MHBauer · 2019-04-08T23:17:09Z

no, I missed an 'l', oops /lgtm

…

On 4/8/19 15:20:28, Jonathan Berkhahn wrote: @MHBauer <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_MHBauer&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=Cm9jVP2dXgRon2ECJNfDtMtqTLawnzcAqmUnlVnKjkY&e=> looks like your LGTM didn't take, could you re /lgtm this? — You are receiving this because you were mentioned. 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_2582-23issuecomment-2D481029512&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=fdnd83nZuo1up7fijXZjKZRJK76rgbwXUdUlqWsmqP4&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMuDAYSEAcfOtoAiBzxzlo4nYdA6Fmw2ks5ve8CsgaJpZM4bxicv&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=96H5OORWvN_sbAkajKoIszLhy_hBmDo1AO16NsRxqH0&e=>.

MHBauer · 2019-04-08T23:18:13Z

/woof

…

On 4/8/19 15:20:28, Jonathan Berkhahn wrote: @MHBauer <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_MHBauer&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=Cm9jVP2dXgRon2ECJNfDtMtqTLawnzcAqmUnlVnKjkY&e=> looks like your LGTM didn't take, could you re /lgtm this? — You are receiving this because you were mentioned. 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_2582-23issuecomment-2D481029512&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=fdnd83nZuo1up7fijXZjKZRJK76rgbwXUdUlqWsmqP4&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMuDAYSEAcfOtoAiBzxzlo4nYdA6Fmw2ks5ve8CsgaJpZM4bxicv&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=96H5OORWvN_sbAkajKoIszLhy_hBmDo1AO16NsRxqH0&e=>.

k8s-ci-robot · 2019-04-08T23:18:14Z

@MHBauer:

In response to this:

/woof

On 4/8/19 15:20:28, Jonathan Berkhahn wrote:

@MHBauer
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_MHBauer&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=Cm9jVP2dXgRon2ECJNfDtMtqTLawnzcAqmUnlVnKjkY&e=
looks like your LGTM didn't take, could you re /lgtm this?

—
You are receiving this because you were mentioned.
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_2582-23issuecomment-2D481029512&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=fdnd83nZuo1up7fijXZjKZRJK76rgbwXUdUlqWsmqP4&e=,
or mute the thread
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMuDAYSEAcfOtoAiBzxzlo4nYdA6Fmw2ks5ve8CsgaJpZM4bxicv&d=DwMCaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=gSVznMk4DixLVQ8USuNSH0RuT_4psswDPWAf-45M48c&m=ORPM8Bste7vfHngefKkMh_UOGCvjz2uptdo4Q7Uws_k&s=96H5OORWvN_sbAkajKoIszLhy_hBmDo1AO16NsRxqH0&e=.

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.

jboyd01

I'm kind of surprised the namespaced scoped describe class test works since the namespace scope doesn't work from the command line (flag not recognized) (see below)....

At md/svcat/class/describe_cmd.go:114

	opts := servicecatalog.ScopeOptions{Scope: servicecatalog.AllScope}
	plans, err := c.App.RetrievePlans(class.GetName(), opts)
	if err != nil {
		return err
	}

I think if the scope is namespace we should set namespace for the plan scope as well. Probably applies to more then just plans. IE all describe operations pull up instances/classes/plans right? we should apply the same scope to all describe commands. Maybe this is best done in a follow up, if you agree would you mind creating an issue? Or let me know and I can create.

jboyd01 · 2019-04-09T17:36:00Z

cmd/svcat/class/describe_cmd.go

 	)
+	describeCmd.AddScopedFlags(cmd.Flags(), true)
+


Missing describeCmd.AddNamespaceFlags(cmd.Flags(), false). Running svcat describe class --help doesn't show the -n or --namespace option and trying to use it I get errors about unknown flag

Yeah, the reason the test passes is the actual bit that compiles the command and parses junk is inside the cobra framework, so I can't test it. I have to manually reach in and set the fields.

As for filtering the plans by namespace, that shouldn't be necessary because we're filtering by the exact k8s name of the parent class (filtered through the pkg/svcat interface so it doesn't matter if it's a ServiceClass or ClusterServiceClass).

As for filtering the plans by namespace, that shouldn't be necessary because we're filtering by the exact k8s name of the parent class (filtered through the pkg/svcat interface so it doesn't matter if it's a ServiceClass or ClusterServiceClass).

I was doing some poking a week ago with this review as a non-admin user and specified a single namespace on the describe class command. I got errors indicating I didn't have privs to get cluster scoped plans. So I think this is a real issue, but I'll have to wait 'til next time I have an environment up and time to reproduce. I'm fine if we address that in a separate issue.

oh huh, that is interesting. Although I think that's a larger problem that the scope of this PR, because doesn't that mean you wouldn't be able to see cluster plans to know what they're called to even provision one?

jboyd01 · 2019-04-09T18:41:03Z

cmd/svcat/plan/get_cmd.go

@@ -112,10 +112,6 @@ func (c *getCmd) Validate(args []string) error {
 }

 func (c *getCmd) Run() error {
-	fmt.Println("KUBENAME: ", c.kubeName)


I saw this output in my last built svcat - glad you are removing this, I wasn't aware we had actually checked this in like this.

jberkhahn · 2019-04-15T23:29:09Z

@MHBauer need to review again
@jboyd01 addressed your things

MHBauer · 2019-04-16T17:14:37Z

test fails
/lgtm

MHBauer · 2019-04-16T17:15:47Z

real failure, need to update the goldens and the completions

jboyd01 · 2019-04-16T20:09:50Z

/approve

k8s-ci-robot · 2019-04-16T20:09:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

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~~ [jboyd01]
~~pkg/svcat/OWNERS~~ [jboyd01]

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

jberkhahn · 2019-04-16T21:29:28Z

updated the golden files

MHBauer · 2019-04-17T18:20:25Z

/lgtm

jberkhahn · 2019-04-17T21:06:03Z

/test pull-service-catalog-xbuild

beyond the scope of this PR

…rnetes-retired#2582)

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

k8s-ci-robot requested review from eriknelson and jeremyrickard March 13, 2019 23:08

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

jberkhahn force-pushed the namespace_describe_classes branch 8 times, most recently from 87f9279 to 72ab07b Compare March 14, 2019 22:27

k8s-ci-robot requested review from jboyd01 and MHBauer March 14, 2019 23:05

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2019

jberkhahn commented Apr 1, 2019

View reviewed changes

MHBauer reviewed Apr 1, 2019

View reviewed changes

jberkhahn force-pushed the namespace_describe_classes branch from 72ab07b to b501f37 Compare April 2, 2019 22:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2019

jberkhahn force-pushed the namespace_describe_classes branch 2 times, most recently from f5e570b to 2b71b96 Compare April 2, 2019 22:38

MHBauer self-requested a review April 3, 2019 00:34

MHBauer approved these changes Apr 3, 2019

View reviewed changes

k8s-ci-robot assigned MHBauer Apr 8, 2019

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

jboyd01 previously requested changes Apr 9, 2019

View reviewed changes

jberkhahn force-pushed the namespace_describe_classes branch from 2b71b96 to bd66bb9 Compare April 15, 2019 23:28

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

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

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

svcat describe and get classes now support namespaced resources

bdf851d

jberkhahn force-pushed the namespace_describe_classes branch from bd66bb9 to bdf851d Compare April 16, 2019 21:29

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

jberkhahn requested review from jboyd01 and MHBauer April 16, 2019 21:38

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

k8s-ci-robot merged commit 7e3e171 into kubernetes-retired:master Apr 17, 2019

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

svcat describe and get classes now support namespaced resources (kube…

8a7451f

…rnetes-retired#2582)

cblecker unassigned MHBauer Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

svcat describe and get classes now support namespaced resources #2582

svcat describe and get classes now support namespaced resources #2582

jberkhahn commented Mar 13, 2019

jberkhahn commented Mar 14, 2019

jberkhahn Apr 1, 2019

MHBauer Apr 3, 2019

jberkhahn Apr 1, 2019

MHBauer left a comment

MHBauer Mar 25, 2019

MHBauer Apr 1, 2019

jberkhahn commented Apr 3, 2019

MHBauer left a comment

MHBauer Apr 3, 2019

jberkhahn commented Apr 8, 2019

MHBauer commented Apr 8, 2019 via email

MHBauer commented Apr 8, 2019 via email

k8s-ci-robot commented Apr 8, 2019

jboyd01 left a comment

jboyd01 Apr 9, 2019

jberkhahn Apr 15, 2019

jboyd01 Apr 16, 2019

jberkhahn Apr 17, 2019

jboyd01 Apr 9, 2019

jberkhahn commented Apr 15, 2019

MHBauer commented Apr 16, 2019

MHBauer commented Apr 16, 2019

jboyd01 commented Apr 16, 2019

k8s-ci-robot commented Apr 16, 2019

jberkhahn commented Apr 16, 2019

MHBauer commented Apr 17, 2019

jberkhahn commented Apr 17, 2019

svcat describe and get classes now support namespaced resources #2582

svcat describe and get classes now support namespaced resources #2582

Conversation

jberkhahn commented Mar 13, 2019

jberkhahn commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jberkhahn commented Apr 3, 2019

MHBauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Apr 8, 2019

MHBauer commented Apr 8, 2019 via email

MHBauer commented Apr 8, 2019 via email

k8s-ci-robot commented Apr 8, 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

Choose a reason for hiding this comment

jberkhahn commented Apr 15, 2019

MHBauer commented Apr 16, 2019

MHBauer commented Apr 16, 2019

jboyd01 commented Apr 16, 2019

k8s-ci-robot commented Apr 16, 2019

jberkhahn commented Apr 16, 2019

MHBauer commented Apr 17, 2019

jberkhahn commented Apr 17, 2019