Add basic Register broker command to Svcat #2162

jberkhahn · 2018-06-27T23:41:57Z

This command will allow users to add a broker to service catalog

luksa · 2018-06-29T06:18:56Z

cmd/svcat/broker/register_cmd.go

+		Use:   "register NAME --url URL",
+		Short: "Registers a new broker with service catalog",
+		Example: command.NormalizeExamples(`
+		svcat register mysqlbroker --url http://mysqlbroker.com


I believe this needs to be svcat register broker mysqlbroker ..., so it's aligned with svcat sync broker mysqlbroker

I'm not sure about that. I would think register would be like provision and bind, neither of which require you to specify the object types they create. @carolynvs thoughts?

I'm with Jonathan on that, for our domain specific verbs that only apply to a single resource type (in this case a broker), I would prefer that we are consistent and not require the redundant noun.

jberkhahn · 2018-07-05T21:31:49Z

Ok, this should be ready to be merged now.

Most of the size is from junk I had to rewrite to turn pkg/svcat/service-catalog into an interface. Everything it does is still the same, it's just specified in an interface so it can be faked out in cmd/svcat for the unit tests.

staebler

make test-update-goldenfiles fails

Fixed the new test suit. make test-update-goldenfiles works

MHBauer · 2018-07-05T22:26:15Z

pkg/svcat/svcat.go

-	*servicecatalog.SDK
-
+	//*servicecatalog.SDK
+	SDK servicecatalog.SvcatAPI


why not keep this as an embed and avoid the App.SDK.fn calls change above?

Other than my dislike of inlining in general? Not really I suppose. I'll revert that bit.

MHBauer · 2018-07-05T22:31:33Z

pkg/svcat/service-catalog/sdk.go

+	RetrieveSecretByBinding(*apiv1beta1.ServiceBinding) (*apicorev1.Secret, error)
+
+	ServerVersion() (*version.Info, error)
+}


Not being sure how this interface is going to be used, I wonder if it best split up into multiple sub interfaces, approximately how it is split by newlines.

And then what? Have the Svcat app type inline multiple interfaces? Seems like it would be more of a mess to fake, esp. considering you would have to update them individually with Counterfeiter.

Having one bighuge interface does seem kind of messy, but I don't think splitting it up is actually a better alternative.

Exactly so. If this isn't intended to be used as a nicer version of a client in any way, then we can leave it, otherwise I think it would be nice to have a binder and a provisioner, or a broker client and a plan client, etc.
I'm fine with it as is. Like I said, I haven't thought about how the interface is to be used.

I am fine with a mega interface for now. If there's a need to break it down further, let's tackle that then.

MHBauer · 2018-07-06T19:20:12Z

pkg/svcat/service-catalog/binding.go

 }

 // IsBindingFailed returns if the instance is in the Failed status.
 func (sdk *SDK) IsBindingFailed(binding *v1beta1.ServiceBinding) bool {
-	return sdk.BindingHasStatus(binding, v1beta1.ServiceBindingConditionFailed)
+	return sdk.bindingHasStatus(binding, v1beta1.ServiceBindingConditionFailed)
 }

 // BindingHasStatus returns if the instance is in the specified status.


returns true

i didn't write that as part of this PR, but thanks

MHBauer · 2018-07-06T19:21:54Z

pkg/svcat/service-catalog/broker.go

+			},
+		},
+	}
+	result, err := sdk.ServiceCatalog().ClusterServiceBrokers().Create(request)


is auth and stuff handled inside of here or is it future work?

Authenticated brokers is not covered as part of this PR

Fixed.

MHBauer

LGTM from me about the parts I understand.
/lgtm
I'll let @carolynvs do the approval.

carolynvs

I'm not done yet will finish tonight after dinner.

carolynvs · 2018-07-12T00:07:55Z

pkg/svcat/service-catalog/sdk.go

 	"k8s.io/client-go/kubernetes"
 	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 )

+// SvcatAPI is an interface containing the variuos actions in the svcat pkg lib
+// This interface is then faked with Counterfeiter for the cmd/svcat unit tests


Please add a blurb to docs/devguide.md explaining how to install and use Counterfeiter to perform this task.

carolynvs · 2018-07-12T00:11:30Z

cmd/svcat/broker/register_cmd_test.go

+			Expect(*cmd).NotTo(BeNil())
+			Expect(cmd.Use).To(Equal("register NAME --url URL"))
+			Expect(cmd.Short).To(Equal("Registers a new broker with service catalog"))
+			Expect(cmd.Example).To(Equal("  svcat register mysqlbroker --url http://mysqlbroker.com"))


Can we switch from Equals to Contains so that we aren't so whitespace sensitive?

carolynvs · 2018-07-12T00:14:47Z

pkg/svcat/service-catalog/binding.go

 }

 // BindingHasStatus returns if the instance is in the specified status.
-func (sdk *SDK) BindingHasStatus(binding *v1beta1.ServiceBinding, status v1beta1.ServiceBindingConditionType) bool {
+func (sdk *SDK) bindingHasStatus(binding *v1beta1.ServiceBinding, status v1beta1.ServiceBindingConditionType) bool {


carolynvs · 2018-07-12T14:14:49Z

pkg/svcat/service-catalog/sdk.go

 	"k8s.io/client-go/kubernetes"
 	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 )

+// SvcatAPI is an interface containing the variuos actions in the svcat pkg lib
+// This interface is then faked with Counterfeiter for the cmd/svcat unit tests
+type SvcatAPI interface {


Can we come up with a different name for this that doesn't include "API"? The reason I went with SDK on the wrapper that I created was that it really wasn't a blind api client like the regenerated go client already is.

carolynvs

Just some small nits and then this is good to go.

jberkhahn · 2018-07-12T18:14:32Z

@MHBauer @carolynvs

Moved some things around a bit. Renamed the pkg/svcat interface to SvcatClient, because it's Svcat's wrapper around the generated client. Is that a better name?

Added a blurb about counterfeiter to docs/devguide.md as well.

MHBauer · 2018-07-12T22:29:21Z

docs/devguide.md

+Counterfeiter by running `go get http://github.com/maxbrunsfeld/counterfeiter && go install http://github.com/maxbrunsfeld/countefeiter`.
+Then regenerate the fake with `counterfeiter ./path/to/dir/with/interface InterfaceName`. You may have to manually paste the boilerplate
+comment into the fakes.
+


is the version of counterfeiter relevant to mention? I know we had some issues to work through.

MHBauer · 2018-07-12T22:29:35Z

docs/devguide.md

+Certain tests use fakes generated with [Counterfeiter](http://github.com/maxbrunsfeld/counterfeiter). If you add a method
+to an interface (such as SvcatAPI in pkg/svcat/service-catalog) you may need to regenerate the fake. You can install
+Counterfeiter by running `go get http://github.com/maxbrunsfeld/counterfeiter && go install http://github.com/maxbrunsfeld/countefeiter`.
+Then regenerate the fake with `counterfeiter ./path/to/dir/with/interface InterfaceName`. You may have to manually paste the boilerplate


good mention of the boilerplate.

MHBauer

I'll wait for carolyn to get off the plane, but seems fine still. I like the added docs.
/lgtm

carolynvs

Just one small change (not all the renames made it in) and then this is ready to merge.

carolynvs · 2018-07-13T20:58:18Z

pkg/svcat/svcat.go

 	"github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog"
 	k8sclient "k8s.io/client-go/kubernetes"
 )

 // App is the underlying application behind the svcat cli.
 type App struct {
-	*servicecatalog.SDK
-
+	// SvcatAPI is the interface for performing actions against the service-catalog apiserver


SvcatClient

carolynvs · 2018-07-13T20:58:55Z

docs/devguide.md

@@ -310,6 +310,13 @@ Keep in mind that golden files help catch errors when the output unexpectedly ch
 It's up to you to judge when you should run the tests with -update, 
 and to diff the changes in the golden file to ensure that the new output is correct.

+### Counterfeiter
+Certain tests use fakes generated with [Counterfeiter](http://github.com/maxbrunsfeld/counterfeiter). If you add a method
+to an interface (such as SvcatAPI in pkg/svcat/service-catalog) you may need to regenerate the fake. You can install


SvcatClient instead of SvcatAPI

- refactor pkg/svcat to use an interface - begin adding unit tests to cmd/svcat w/ faked out pkg interface

carolynvs

LGTM

MHBauer

/LGTM
LGTM
/approve
go for merge

k8s-ci-robot · 2018-07-16T21:01:49Z

[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:

~~OWNERS~~ [MHBauer]

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2018

k8s-ci-robot requested review from luksa and n3wscott June 27, 2018 23:42

luksa reviewed Jun 29, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register branch from cca0867 to eadabd5 Compare July 3, 2018 22:59

jberkhahn mentioned this pull request Jul 4, 2018

Add unit test for cmd/svcat/broker/describe_cmd.go #2179

Merged

jberkhahn force-pushed the svcat_register branch from eadabd5 to 6b35097 Compare July 4, 2018 01:06

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 4, 2018

jberkhahn force-pushed the svcat_register branch from 6b35097 to 9a88cfc Compare July 5, 2018 21:22

jberkhahn removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2018

staebler previously requested changes Jul 5, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register branch from 9a88cfc to cb41072 Compare July 5, 2018 21:59

jberkhahn changed the title ~~WIP: Issue 1807 Add Register broker command to Svcat~~ Add basic Register broker command to Svcat Jul 5, 2018

MHBauer reviewed Jul 5, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register branch 7 times, most recently from a1aac87 to 1757979 Compare July 6, 2018 19:01

MHBauer reviewed Jul 6, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register branch from 1757979 to 506e0e8 Compare July 6, 2018 19:22

jberkhahn force-pushed the svcat_register branch from c69ae0f to f2ab8b6 Compare July 11, 2018 22:37

jberkhahn force-pushed the svcat_register branch from f2ab8b6 to 162b67e Compare July 11, 2018 22:49

k8s-ci-robot assigned MHBauer Jul 11, 2018

MHBauer approved these changes Jul 11, 2018

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018

carolynvs reviewed Jul 12, 2018

View reviewed changes

carolynvs suggested changes Jul 12, 2018

View reviewed changes

jberkhahn force-pushed the svcat_register branch from 162b67e to 1183b97 Compare July 12, 2018 18:12

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2018

MHBauer reviewed Jul 12, 2018

View reviewed changes

MHBauer approved these changes Jul 12, 2018

View reviewed changes

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2018

carolynvs suggested changes Jul 13, 2018

View reviewed changes

Add basic register broker command to svcat

1e2b815

- refactor pkg/svcat to use an interface - begin adding unit tests to cmd/svcat w/ faked out pkg interface

jberkhahn force-pushed the svcat_register branch from 1183b97 to 1e2b815 Compare July 16, 2018 18:32

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

carolynvs approved these changes Jul 16, 2018

View reviewed changes

carolynvs added the LGTM1 label Jul 16, 2018

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

MHBauer approved these changes Jul 16, 2018

View reviewed changes

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

jberkhahn added the LGTM2 label Jul 16, 2018

jberkhahn merged commit d4b2273 into kubernetes-retired:master Jul 16, 2018

MHBauer mentioned this pull request Jul 19, 2018

v0.1.26 Release #2213

Merged

cblecker unassigned MHBauer Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic Register broker command to Svcat #2162

Add basic Register broker command to Svcat #2162

jberkhahn commented Jun 27, 2018 •

edited

Loading

luksa Jun 29, 2018

jberkhahn Jun 29, 2018 •

edited

Loading

carolynvs Jul 11, 2018

jberkhahn commented Jul 5, 2018

staebler left a comment

MHBauer Jul 5, 2018

jberkhahn Jul 5, 2018

MHBauer Jul 5, 2018

jberkhahn Jul 5, 2018

MHBauer Jul 6, 2018

carolynvs Jul 12, 2018

MHBauer Jul 12, 2018

MHBauer Jul 6, 2018

jberkhahn Jul 6, 2018 •

edited

Loading

MHBauer Jul 6, 2018

jberkhahn Jul 6, 2018

MHBauer left a comment

carolynvs left a comment

carolynvs Jul 12, 2018

carolynvs Jul 12, 2018

carolynvs Jul 12, 2018

carolynvs Jul 12, 2018

carolynvs left a comment

jberkhahn commented Jul 12, 2018

MHBauer Jul 12, 2018

MHBauer Jul 12, 2018

MHBauer left a comment

carolynvs left a comment

carolynvs Jul 13, 2018

carolynvs Jul 13, 2018

jberkhahn Jul 16, 2018

carolynvs left a comment

MHBauer left a comment

k8s-ci-robot commented Jul 16, 2018

Add basic Register broker command to Svcat #2162

Add basic Register broker command to Svcat #2162

Conversation

jberkhahn commented Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

jberkhahn Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Jul 5, 2018

staebler 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 Jul 6, 2018 • edited Loading

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

carolynvs 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

carolynvs left a comment

Choose a reason for hiding this comment

jberkhahn commented Jul 12, 2018

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

carolynvs 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

carolynvs left a comment

Choose a reason for hiding this comment

MHBauer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 16, 2018

jberkhahn commented Jun 27, 2018 •

edited

Loading

jberkhahn Jun 29, 2018 •

edited

Loading

jberkhahn Jul 6, 2018 •

edited

Loading