-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Write APIs CLI commands #64
Conversation
Tested 1-2 commands, will continue later:
Get:
Poll:
It seems natural to say
and I have to call
|
internal/cmd/instance/provision.go
Outdated
return err | ||
} | ||
if len(plans.ServicePlans) != 1 { | ||
return fmt.Errorf("exactly one service plan with name %s for offering with id %s ecpected", pi.planName, pi.instance.ServiceID) |
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.
typo "ecpected"
@@ -56,13 +55,9 @@ func (gb *GetInstanceCmd) Run() error { | |||
return nil | |||
} | |||
|
|||
resultInstances := &types.ServiceInstances{} | |||
resultInstances := &types.ServiceInstances{Vertical: true} |
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.
Can we use types.ServiceInstance
and remove the Vertical property of the plural type?
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.
There is a case, where you can have more than one instance with the same name. However this cases are rare and if there is such a situation the instances probably wouldn't be too much. That's why decided to make it vertical, since most of the time the instance will be just one and the readability would be significantly improved.
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.
If there 2 instances and it looks good then it is ok for me
For me failed deprovision operations is returned, are you sure you use latest SM and everything is dep ensured, since this functionality become available in SM this morning.
The concept of If you have any ideas I am willing to enhance the commands in different PRs, if this will speed up the review process. |
@DimitarPetrov For debugging purposes it would make sense to have the option to get the operation in such case. For the happy path (failure to provision is still a happy path), this seems like an overhead and unnecessary information. Provision can tell me something like |
@dpanayotov |
internal/cmd/binding/unbind.go
Outdated
return fmt.Errorf("more than one service instance with name %s found. Use --id flag to specify id of the binding to be deleted", ubc.instanceName) | ||
} | ||
|
||
bindingsToDelte, err := ubc.Client.ListBindings(&query.Parameters{ |
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.
Minor: typo in bindingsToDelte
@@ -39,13 +32,13 @@ var _ = Describe("Register Broker Command test", func() { | |||
command = NewRegisterBrokerCmd(context) | |||
}) | |||
|
|||
validRegisterBrokerExecution := func(args []string) *cobra.Command { | |||
validAsyncRegisterBrokerExecution := func(args []string, location string) *cobra.Command { |
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.
Minor: in all other similar methods to this one, the location parameter always comes first. For consistency reasons, is it a good idea to switch the arguments here?
internal/cmd/commander.go
Outdated
sync, errSyncFlag := c.Flags().GetBool("sync") | ||
|
||
if errAsyncFlag == nil && errSyncFlag == nil { | ||
return fmt.Errorf("ambiguas flags provided sync and async") |
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.
Minor: typo ambiguas
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.
Looks good to me.
What is included
See examples below!
Examples