Add svcat command to create user-defined cluster-scoped classes #2190

artmello · 2018-07-11T20:33:32Z

Closes: #2111

@carolynvs this is an initial work to add command to svcat for creating user-defined classes. By now, it should handle only cluster-scoped classes. It is for sure missing some svcat command level tests and I am unsure what should be the return to the user in case of success: display created class name; display created class uuid; display all class info like 'get' command; some other thing. If you have sometime to review the current approach and suggest command output I can work on missing tests.

carolynvs · 2018-07-11T20:35:57Z

Woooooooooooooooo!

carolynvs · 2018-07-11T20:36:53Z

/ok-to-test

carolynvs

This is looking great so far!

carolynvs · 2018-07-11T20:50:01Z

cmd/svcat/class/create_cmd.go

+		PreRunE: command.PreRunE(createCmd),
+		RunE:    command.RunE(createCmd),
+	}
+	cmd.Flags().StringVarP(


nit: this could be a single line or maybe just this instead

cmd.Flags().StringVarP(&createCmd.from, "from", "f", "", "Name from an existing class that will be copied")

Just letting you know that splitting it up per line isn't a style that you have to use, in case you copied it from elsewhere in our code.

carolynvs · 2018-07-11T20:52:28Z

cmd/svcat/class/create_cmd.go

+		return fmt.Errorf("new class name should be provided")
+	}
+
+	if c.from == "" {


I have a cobra trick for you! You can do this instead when you define the flag and then cobra will enforce this for you.

cmd.MarkFlagRequired("from")

carolynvs · 2018-07-11T20:53:07Z

cmd/svcat/class/create_cmd.go

+
+	class.Spec.ExternalName = c.name
+
+	_, err = c.App.CreateClass(class)


carolynvs · 2018-07-11T21:00:03Z

pkg/svcat/service-catalog/class_test.go

@@ -207,4 +207,38 @@ var _ = Describe("Class", func() {
 			Expect(actions[0].(testing.GetActionImpl).Name).To(Equal(fakeClassName))
 		})
 	})
+	Describe("CreateClassByName", func() {


I am not sure if this is the right name for the test, no? The function takes in the entire class resource, not just a name. How about CreateClass instead?

carolynvs · 2018-07-11T21:02:29Z

pkg/svcat/service-catalog/class_test.go

+	Describe("CreateClassByName", func() {
+		It("Calls the generated v1beta1 create method with the passed in class", func() {
+			realClient := &fake.Clientset{}
+			realClient.AddReactor("create", "clusterserviceclasses", func(action testing.Action) (bool, runtime.Object, error) {


I don't believe that you need to define a Create Reactor here? Pretty sure that the default test fake handles this for you already. Does it still work if you remove lines 213-215?

Yeah you don't need to manually add a reactor for the simple case like here. See the test case for provision instance here.

jberkhahn · 2018-07-11T21:06:36Z

Yo!

Neat PR, definitely interested in this. I'm in the middle of refactoring the svcat cmd/pkg interface (as well as adding some cmd/svcat unit tests that you could crib from) here: #2162.

Will take a look and comment.

jberkhahn · 2018-07-11T21:13:00Z

cmd/svcat/class/create_cmd.go

+		return err
+	}
+
+	class.Spec.ExternalName = c.name


Does this actually work? Wouldn't it barf because you're trying to create a new class with the same UID as the original class? Also, the Status in the new class you're trying to create would be fully inflated, which might cause strange things to happen.

That should work. K8s handles generating a new uid, and will ignore status because it's a subresource. Either way we should add some tests to verify that assumption.

jberkhahn · 2018-07-11T21:14:06Z

pkg/svcat/service-catalog/class.go

@@ -77,3 +77,13 @@ func (sdk *SDK) RetrieveClassByPlan(plan *v1beta1.ClusterServicePlan,

 	return class, nil
 }
+
+// CreateClass returns new created class
+func (sdk *SDK) CreateClass(class *v1beta1.ClusterServiceClass) (*v1beta1.ClusterServiceClass, error) {


Are we going to come up with a unique verb for creating a class like register/provision/bind for brokers/instances/bindings?

I didn't change this one on latest commit. Since I don't have any strong opinion about it, I will wait for a decision :)

I wasn't planning on it, no.

Create class seems fine, then.

jberkhahn · 2018-07-11T21:17:37Z

pkg/svcat/service-catalog/class_test.go

@@ -207,4 +207,38 @@ var _ = Describe("Class", func() {
 			Expect(actions[0].(testing.GetActionImpl).Name).To(Equal(fakeClassName))
 		})
 	})
+	Describe("CreateClassByName", func() {


Ginkgo tests yeah!

Just one caveat, this describe should be "CreateClass" as that's the method you created, and could have another describe inside called "CreateClassByName". Or maybe actually all the describes should be Context(method name) and there should be Describe(test case situation) inside them. Hmmm.

jberkhahn · 2018-07-11T21:22:19Z

pkg/svcat/service-catalog/class_test.go

+	Describe("CreateClassByName", func() {
+		It("Calls the generated v1beta1 create method with the passed in class", func() {
+			realClient := &fake.Clientset{}
+			realClient.AddReactor("create", "clusterserviceclasses", func(action testing.Action) (bool, runtime.Object, error) {


Yeah you don't need to manually add a reactor for the simple case like here. See the test case for provision instance here.

- Add 'create' command to svcat main.go; - Changes Cobra command declaration; - Improve new test on pkg/svcat/service-catalog/class_test.go - Fix data provided to new generated class on cmd/svcat/class/create_cmd.go - Print success message to the user after creating a class - Add unittest to cmd/svcat/svcat_test.go - Update goldenfiles

artmello · 2018-07-14T03:03:15Z

@carolynvs / @jberkhahn thanks a lot for reviewing this, hopefully latest commit address most of previous comments :)

carolynvs · 2018-07-16T19:43:40Z

cmd/svcat/class/create_cmd.go

@@ -73,12 +67,22 @@ func (c *createCmd) Run() error {
 		return err
 	}

-	class.Spec.ExternalName = c.name
+	newClass := &v1beta1.ClusterServiceClass{


I'm confused about why you changed this part? Previously, it would copy the spec of the original class, and save it with a new name which was the desired behavior. With this change, the new class is missing key information from the original and it wouldn't be usable.

My vote is for going back to what you had and making sure through tests that it does what we need without unwanted side effects (if that was the concern that made you change it?)

Just realized that this was in response to one of Jonathan's comments above. I replied there but I believe we can go back to what you had before (I spoke with him in person about this last week) and just make sure that we have enough tests to verify that it works as we hoped, without odd side-effects.

carolynvs · 2018-07-16T19:43:59Z

cmd/svcat/class/create_cmd.go

 	if err != nil {
 		return err
 	}

+	output.WriteCreatedResourceName(c.Output, createdClass.Spec.ExternalName)


carolynvs · 2018-07-16T19:44:56Z

cmd/svcat/main.go

+func newCreateCmd(cxt *command.Context) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "create",
+		Short: "Create an user-defined resource",


nit: change "an" to "a"

carolynvs · 2018-07-16T19:48:02Z

pkg/svcat/service-catalog/class_test.go

 			Expect(actions[0].Matches("create", "clusterserviceclasses")).To(BeTrue())
+			objectFromRequest := actions[0].(testing.CreateActionImpl).Object.(*v1beta1.ClusterServiceClass)


Same question as above, this is a change in behavior and I'm not sure what's driving it?

… cmd

jberkhahn

My PR just went in to refactor the cmd/pkg into an interface so it can be faked out. Need to fix this so it works on top of the new code. Also, add a unit test for the cmd layer. Hopefully it shouldn't be too hard to figure out, should be pretty close to the one I added for cmd/svcat/register_broker_cmd.go. You will need to add CreateClass to the interface and regenerate the fakes with (Counterfeiter)[https://github.com/maxbrunsfeld/counterfeiter]

If you need any help, message me on the Kube slack and I can walk you through it.

jberkhahn · 2018-07-16T21:44:10Z

cmd/svcat/output/output.go

@@ -53,6 +53,11 @@ func formatStatusFull(condition string, conditionStatus v1beta1.ConditionStatus,
 	return fmt.Sprintf("%s - %s @ %s", status, message, timestamp.UTC())
 }

+// WriteCreatedResourceName prints the name of a created resource
+func WriteCreatedResourceName(w io.Writer, resourceName string) {


Is there a reason we don't have anything suitable for this in output/class.go?

I don't understand the question/suggestion?

For other resources when we create them (like a binding), we have just printed out the details (basically the output of describe). We haven't had svcat create class resources yet so this is net new. Once we start allowing creating plans too, this function will be resued.

No? We couldn't use something like WriteClass or WriteClassDetails?

Oh! I didn't get that you are were suggesting that we output WriteClassDetails instead of just the name. 😊 Yeah that would be fine with me either way.

MHBauer · 2018-07-17T16:27:13Z

/assign @carolynvs

carolynvs

What you have so far looks good to me, but since it's a new feature it will also need docs.

Can you add a section to https://github.com/kubernetes-incubator/service-catalog/blob/master/docs/cli.md called "Create a custom class" and show an example command and output?

carolynvs · 2018-07-19T15:27:57Z

Looks like your golden files are out of date (or there is a bug in one of the tests) based on the failed build. https://svc-cat.io/docs/devguide/#golden-files explains how to regenerate the golden files.

carolynvs · 2018-07-23T16:13:08Z

@artmello Let me know if you need help getting the build passing, I have time this week to help get this PR ready to merge. 😀

- Change Name instead of ExternalName

- Change Name instead of ExternalName - Add CreateClass to sdk interface

artmello · 2018-07-26T10:37:26Z

@carolynvs / @jberkhahn thanks a lot for the review so far, hopefully all your comments are addressed with latest commits.

carolynvs

/lgtm

ZOMG I'm so excited! I can't wait to start building off of it! 💃 🕺

carolynvs · 2018-07-27T17:28:05Z

cmd/svcat/class/create_cmd.go

+
+// Run calls out to the pkg lib to create the class and displays the output
+func (c *CreateCmd) Run() error {
+	class, err := c.App.RetrieveClassByName(c.From)


FYI, there will need to be a follow-on PR to make this work for namespaced classes. That feature is super new so it doesn't need to be in this PR. Just giving you a heads up in case you want to pick up more related stuff after this PR is merged! 😀

jberkhahn · 2018-07-30T20:28:32Z

/lgtm

jberkhahn · 2018-07-30T20:28:38Z

/approve

k8s-ci-robot · 2018-07-30T20:31:51Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: 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

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

Add svcat command to create user-defined cluster-scoped classes

1424086

k8s-ci-robot added 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 Jul 11, 2018

k8s-ci-robot requested review from jberkhahn and kikisdeliveryservice July 11, 2018 20:33

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 Jul 11, 2018

carolynvs self-requested a review July 11, 2018 20:36

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2018

carolynvs suggested changes Jul 11, 2018

View reviewed changes

jberkhahn suggested changes Jul 11, 2018

View reviewed changes

carolynvs suggested changes Jul 16, 2018

View reviewed changes

artmello added 4 commits July 16, 2018 17:12

Merge remote-tracking branch 'origin/master' into user_defined_classes

ec0300a

Fix help messaging language

c6c49b6

Use class data got from RetrieveClassByName to pass to CreateClass in…

0cef4a5

… cmd

Fix create class unit test on pkg

25141be

jberkhahn suggested changes Jul 16, 2018

View reviewed changes

k8s-ci-robot assigned carolynvs Jul 17, 2018

carolynvs suggested changes Jul 19, 2018

View reviewed changes

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

Merge remote-tracking branch 'origin/master' into user_defined_classes

39e34bf

artmello force-pushed the user_defined_classes branch from f709986 to 39e34bf Compare July 25, 2018 17:15

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

artmello added 8 commits July 25, 2018 14:38

Fix pkg tests for CreateClass

b736b21

- Change Name instead of ExternalName

Fix cmd tests for CreateClass

a30b93b

- Change Name instead of ExternalName - Add CreateClass to sdk interface

Update golden files

a5c5e64

Use WriteClassDetails instead of only printing the name of new class

4049623

Add CreateClass to fake_svcat_client.go

473bb08

Add create_cmd_test.go

a477501

Add entry to docs/cli

4a84457

Add comments to fix lint messages

ff4a0b7

artmello force-pushed the user_defined_classes branch from f4d3cfb to ff4a0b7 Compare July 25, 2018 19:25

Fix wrongly removed function from FakeSvcatClient

1cb3560

carolynvs approved these changes Jul 27, 2018

View reviewed changes

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

carolynvs added the LGTM1 label Jul 27, 2018

jberkhahn approved these changes Jul 30, 2018

View reviewed changes

k8s-ci-robot assigned jberkhahn Jul 30, 2018

jberkhahn added LGTM2 approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 30, 2018

k8s-ci-robot merged commit 3933d12 into kubernetes-retired:master Jul 30, 2018

MHBauer mentioned this pull request Aug 2, 2018

v0.1.28 Release #2256

Merged

carolynvs mentioned this pull request Aug 6, 2018

Create user-defined plans in svcat #2112

Closed


		class.Spec.ExternalName = c.name

		_, err = c.App.CreateClass(class)

		Expect(actions[0].Matches("create", "clusterserviceclasses")).To(BeTrue())
		objectFromRequest := actions[0].(testing.CreateActionImpl).Object.(*v1beta1.ClusterServiceClass)

Add svcat command to create user-defined cluster-scoped classes #2190

Add svcat command to create user-defined cluster-scoped classes #2190

Conversation

artmello commented Jul 11, 2018

carolynvs commented Jul 11, 2018

carolynvs commented Jul 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Jul 11, 2018

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

artmello commented Jul 14, 2018

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

MHBauer commented Jul 17, 2018

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Jul 19, 2018

carolynvs commented Jul 23, 2018

artmello commented Jul 26, 2018

carolynvs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Jul 30, 2018

jberkhahn commented Jul 30, 2018

k8s-ci-robot commented Jul 30, 2018

carolynvs left a comment •

edited

Loading