[v8] Add "--strategy" parameter to "bind-service" command #3654
Conversation
jochenehret
changed the title
| RequiredArgs flag.BindServiceArgs `positional-args:"yes"` | ||
| BindingName flag.BindingName `long:"binding-name" description:"Name to expose service instance to app process with (Default: service instance name)"` | ||
| ParametersAsJSON flag.JSONOrFileWithValidation `short:"c" description:"Valid JSON object containing service-specific configuration parameters, provided either in-line or in a file. For a list of supported configuration parameters, see documentation for the particular service offering."` | ||
| ServiceBindingStrategy flag.ServiceBindingStrategy `long:"strategy" description:"Service binding strategy. Valid values are 'single' (default) and 'multiple'."` |
There was a problem hiding this comment.
Considering we added this new flag, we will need to update the help integration test here:
https://github.com/cloudfoundry/cli/blob/v8/integration/v7/isolated/bind_service_command_test.go#L59-L62
| // Strategy can be "single" (default) or "multiple" | ||
| Strategy BindingStrategyType `jsonry:"strategy,omitempty"` |
There was a problem hiding this comment.
nit: Might be good to add a comment mentioning that empty value related defaulting is done at the backend which defaults to single as you have mentioned. It might not be obvious what the (default) means here.
| CF_NAME bind-service APP_NAME SERVICE_INSTANCE --binding-name BINDING_NAME | ||
| Optionally provide the binding strategy type. Valid options are 'single' (default) and 'multiple'. The 'multiple' strategy allows multiple bindings between the same app and service instance. | ||
| This is useful for credential rotation scenarios. | ||
| CF_NAME bind-service APP_NAME SERVICE_INSTANCE --strategy multiple` |
There was a problem hiding this comment.
we will need to update the help integration test here related to this change.
https://github.com/cloudfoundry/cli/blob/v8/integration/v7/isolated/bind_service_command_test.go#L42-L43
| AppName: cmd.RequiredArgs.AppName, | ||
| BindingName: cmd.BindingName.Value, | ||
| Parameters: types.OptionalObject(cmd.ParametersAsJSON), | ||
| Strategy: cmd.ServiceBindingStrategy.Strategy, |
There was a problem hiding this comment.
nit: May be consider adding a unit test case in command/v7/bind_service_command_test.go that explicitly verifies the strategy parameter:
When("strategy flag is set", func() {
BeforeEach(func() {
setFlag(&cmd, "--strategy", "multiple")
})
It("passes the strategy to the actor", func() {
Expect(fakeActor.CreateServiceAppBindingArgsForCall(0).Strategy).
To(Equal(resources.MultipleBindingStrategy))
})
})
| AppName: cmd.RequiredArgs.AppName, | ||
| BindingName: cmd.BindingName.Value, | ||
| Parameters: types.OptionalObject(cmd.ParametersAsJSON), | ||
| Strategy: cmd.ServiceBindingStrategy.Strategy, |
There was a problem hiding this comment.
Important: Based on the RFC document it seems like this is a new functionality added in CAPI. If that is the case, there has to be a minimum CAPI version required when using this feature right?
Please let me know if I am misunderstanding this but if that is the case, we will need to add a conditional check in the implementation and may be in the API call so that this works with the older version of the CAPI as well as throw meaningful error message when used with older version of CAPI.
There are many places we have done this but one of the example you can see here: https://github.com/cloudfoundry/cli/blob/main/command/v7/buildpacks_command.go#L32-L37
2 participants
Description of the Change
Enhancement for rfc-0040-service-binding-rotation.
Add parameter
strategytobind-servicecommand. Allowed values aresingle(default) andmultiple. Withmultiple, the CLI sends and additional parameter"strategy": "multiple"to the CC v3 API:https://v3-apidocs.cloudfoundry.org/version/3.207.0/index.html#create-a-service-credential-binding
Why Is This PR Valuable?
This PR enhances the CLI so that the multiple service binding feature is enabled for end users. More PRs for other related commands will follow.
Applicable Issues
cloudfoundry/community#1198
How Urgent Is The Change?
Not super urgent.
Other Relevant Parties
Change is backwards compatible, so no regressions expected.