-
Notifications
You must be signed in to change notification settings - Fork 382
Add svcat command to create user-defined cluster-scoped classes #2190
Changes from 2 commits
1424086
3f901e2
ec0300a
c6c49b6
0cef4a5
25141be
39e34bf
b736b21
a30b93b
a5c5e64
4049623
473bb08
a477501
4a84457
ff4a0b7
1cb3560
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package class | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/kubernetes-incubator/service-catalog/cmd/svcat/command" | ||
"github.com/kubernetes-incubator/service-catalog/cmd/svcat/output" | ||
"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1" | ||
"github.com/spf13/cobra" | ||
) | ||
|
||
type createCmd struct { | ||
*command.Context | ||
name string | ||
from string | ||
} | ||
|
||
// NewCreateCmd builds a "svcat create class" command | ||
func NewCreateCmd(cxt *command.Context) *cobra.Command { | ||
createCmd := &createCmd{Context: cxt} | ||
cmd := &cobra.Command{ | ||
Use: "class [NAME] --from [EXISTING_NAME]", | ||
Short: "Copies an existing class into a new user-defined cluster-scoped class", | ||
Example: command.NormalizeExamples(` | ||
svcat create class newclass --from mysqldb | ||
`), | ||
PreRunE: command.PreRunE(createCmd), | ||
RunE: command.RunE(createCmd), | ||
} | ||
cmd.Flags().StringVarP(&createCmd.from, "from", "f", "", | ||
"Name from an existing class that will be copied (Required)", | ||
) | ||
cmd.MarkFlagRequired("from") | ||
|
||
return cmd | ||
} | ||
|
||
func (c *createCmd) Validate(args []string) error { | ||
if len(args) <= 0 { | ||
return fmt.Errorf("new class name should be provided") | ||
} | ||
|
||
c.name = args[0] | ||
|
||
return nil | ||
} | ||
|
||
func (c *createCmd) Run() error { | ||
class, err := c.App.RetrieveClassByName(c.from) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
newClass := &v1beta1.ClusterServiceClass{ | ||
Spec: v1beta1.ClusterServiceClassSpec{ | ||
ClusterServiceBrokerName: class.Spec.ClusterServiceBrokerName, | ||
CommonServiceClassSpec: v1beta1.CommonServiceClassSpec{ | ||
ExternalName: c.name, | ||
Description: class.Spec.Description, | ||
Tags: class.Spec.Tags, | ||
}, | ||
}, | ||
} | ||
|
||
createdClass, err := c.App.CreateClass(newClass) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
output.WriteCreatedResourceName(c.Output, createdClass.Spec.ExternalName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,7 @@ func buildRootCommand(cxt *command.Context) *cobra.Command { | |
cmd.PersistentFlags().StringVar(&opts.KubeContext, "context", "", "name of the kubeconfig context to use.") | ||
cmd.PersistentFlags().StringVar(&opts.KubeConfig, "kubeconfig", "", "path to kubeconfig file. Overrides $KUBECONFIG") | ||
|
||
cmd.AddCommand(newCreateCmd(cxt)) | ||
cmd.AddCommand(newGetCmd(cxt)) | ||
cmd.AddCommand(newDescribeCmd(cxt)) | ||
cmd.AddCommand(instance.NewProvisionCmd(cxt)) | ||
|
@@ -143,6 +144,16 @@ func newSyncCmd(cxt *command.Context) *cobra.Command { | |
return cmd | ||
} | ||
|
||
func newCreateCmd(cxt *command.Context) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "create", | ||
Short: "Create an user-defined resource", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: change "an" to "a" |
||
} | ||
cmd.AddCommand(class.NewCreateCmd(cxt)) | ||
|
||
return cmd | ||
} | ||
|
||
func newGetCmd(cxt *command.Context) *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "get", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we don't have anything suitable for this in output/class.go? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No? We couldn't use something like WriteClass or WriteClassDetails? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
fmt.Fprintf(w, "created %s\n", resourceName) | ||
} | ||
|
||
// WriteDeletedResourceName prints the name of a deleted resource | ||
func WriteDeletedResourceName(w io.Writer, resourceName string) { | ||
fmt.Fprintf(w, "deleted %s\n", resourceName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
created new-class |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we going to come up with a unique verb for creating a class like register/provision/bind for brokers/instances/bindings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't planning on it, no. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Create class seems fine, then. |
||
created, err := sdk.ServiceCatalog().ClusterServiceClasses().Create(class) | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to create class (%s)", err) | ||
} | ||
|
||
return created, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,4 +207,28 @@ var _ = Describe("Class", func() { | |
Expect(actions[0].(testing.GetActionImpl).Name).To(Equal(fakeClassName)) | ||
}) | ||
}) | ||
Describe("CreateClass", func() { | ||
It("Calls the generated v1beta1 create method with the passed in class", func() { | ||
className := "newclass" | ||
nc := &v1beta1.ClusterServiceClass{ObjectMeta: metav1.ObjectMeta{Name: className}} | ||
|
||
class, err := sdk.CreateClass(nc) | ||
|
||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(class).To(Equal(nc)) | ||
actions := svcCatClient.Actions() | ||
Expect(actions[0].Matches("create", "clusterserviceclasses")).To(BeTrue()) | ||
objectFromRequest := actions[0].(testing.CreateActionImpl).Object.(*v1beta1.ClusterServiceClass) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above, this is a change in behavior and I'm not sure what's driving it? |
||
Expect(objectFromRequest.ObjectMeta.Name).To(Equal(className)) | ||
}) | ||
It("Bubbles up errors", func() { | ||
class, err := sdk.CreateClass(sc) | ||
|
||
Expect(class).To(BeNil()) | ||
Expect(err).To(HaveOccurred()) | ||
Expect(err.Error()).Should(ContainSubstring("unable to create class")) | ||
actions := svcCatClient.Actions() | ||
Expect(actions[0].Matches("create", "clusterserviceclasses")).To(BeTrue()) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.