From 18bffc2d9770b10d59c9e387e069fbdf9cad6696 Mon Sep 17 00:00:00 2001 From: Felisia Martini <fmartini@pivotal.io> Date: Tue, 16 Feb 2021 17:25:19 +0000 Subject: [PATCH 1/6] service: --params flag retrieves service instance params [#173917704](https://www.pivotaltracker.com/story/show/173917704) --- actor/v7action/service_instance_details.go | 24 +- .../v7action/service_instance_details_test.go | 210 +++++++++++++----- command/v7/actor.go | 1 + command/v7/service_command.go | 21 +- command/v7/service_command_test.go | 181 ++++++++------- command/v7/v7fakes/fake_actor.go | 85 +++++++ .../v7/isolated/service_command_test.go | 74 +++--- 7 files changed, 401 insertions(+), 195 deletions(-) diff --git a/actor/v7action/service_instance_details.go b/actor/v7action/service_instance_details.go index 0c8a161e033..355696f294a 100644 --- a/actor/v7action/service_instance_details.go +++ b/actor/v7action/service_instance_details.go @@ -71,10 +71,6 @@ func (actor Actor) GetServiceInstanceDetails(serviceInstanceName string, spaceGU serviceInstanceDetails, warnings, err = actor.getServiceInstanceDetails(serviceInstanceName, spaceGUID) return }, - func() (warnings ccv3.Warnings, err error) { - serviceInstanceDetails.Parameters, warnings = actor.getServiceInstanceParameters(serviceInstanceDetails.GUID) - return - }, func() (warnings ccv3.Warnings, err error) { serviceInstanceDetails.SharedStatus, warnings, err = actor.getServiceInstanceSharedStatus(serviceInstanceDetails, spaceGUID) return @@ -97,6 +93,26 @@ func (actor Actor) GetServiceInstanceDetails(serviceInstanceName string, spaceGU return serviceInstanceDetails, Warnings(warnings), nil } +func (actor Actor) GetServiceInstanceParameters(serviceInstanceName string, spaceGUID string) (ServiceInstanceDetails, Warnings, error) { + var serviceInstanceDetails ServiceInstanceDetails + + warnings, err := railway.Sequentially( + func() (warnings ccv3.Warnings, err error) { + serviceInstanceDetails.ServiceInstance, _, warnings, err = actor.getServiceInstanceByNameAndSpace(serviceInstanceName, spaceGUID) + return + }, + func() (warnings ccv3.Warnings, err error) { + serviceInstanceDetails.Parameters, warnings = actor.getServiceInstanceParameters(serviceInstanceDetails.ServiceInstance.GUID) + return + }, + ) + if err != nil { + return ServiceInstanceDetails{}, Warnings(warnings), err + } + + return serviceInstanceDetails, Warnings(warnings), nil +} + func (actor Actor) getServiceInstanceDetails(serviceInstanceName string, spaceGUID string) (ServiceInstanceDetails, ccv3.Warnings, error) { query := []ccv3.Query{ { diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go index 6ec8bff6a61..24c09901c0e 100644 --- a/actor/v7action/service_instance_details_test.go +++ b/actor/v7action/service_instance_details_test.go @@ -73,12 +73,6 @@ var _ = Describe("Service Instance Details Action", func() { nil, ) - fakeCloudControllerClient.GetServiceInstanceParametersReturns( - types.JSONObject{"foo": "bar"}, - ccv3.Warnings{"some-parameters-warning"}, - nil, - ) - fakeCloudControllerClient.GetFeatureFlagReturns( resources.FeatureFlag{Name: "service_instance_sharing", Enabled: true}, ccv3.Warnings{}, @@ -129,7 +123,6 @@ var _ = Describe("Service Instance Details Action", func() { Expect(executionError).NotTo(HaveOccurred()) Expect(warnings).To(ConsistOf( "some-service-instance-warning", - "some-parameters-warning", "some-bindings-warning", )) }) @@ -155,9 +148,7 @@ var _ = Describe("Service Instance Details Action", func() { ServicePlan: resources.ServicePlan{Name: servicePlanName}, ServiceBrokerName: serviceBrokerName, SharedStatus: SharedStatus{}, - Parameters: ServiceInstanceParameters{ - Value: types.JSONObject{"foo": "bar"}, - }, + Parameters: ServiceInstanceParameters{}, BoundApps: []resources.ServiceCredentialBinding{ { Type: resources.AppBinding, @@ -263,53 +254,6 @@ var _ = Describe("Service Instance Details Action", func() { }) }) - Describe("getting the parameters", func() { - It("makes the correct call to get the parameters", func() { - Expect(fakeCloudControllerClient.GetServiceInstanceParametersCallCount()).To(Equal(1)) - Expect(fakeCloudControllerClient.GetServiceInstanceParametersArgsForCall(0)).To(Equal(serviceInstanceGUID)) - }) - - When("getting the parameters fails with an expected error", func() { - BeforeEach(func() { - fakeCloudControllerClient.GetServiceInstanceParametersReturns( - types.JSONObject{}, - ccv3.Warnings{"some-parameters-warning"}, - ccerror.V3UnexpectedResponseError{ - V3ErrorResponse: ccerror.V3ErrorResponse{ - Errors: []ccerror.V3Error{{ - Code: 1234, - Detail: "cannot get parameters reason", - Title: "CF-SomeRandomError", - }}, - }, - }, - ) - }) - - It("does not return an error, but returns warnings and the reason", func() { - Expect(executionError).NotTo(HaveOccurred()) - Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("cannot get parameters reason")) - }) - }) - - When("getting the parameters fails with an another error", func() { - BeforeEach(func() { - fakeCloudControllerClient.GetServiceInstanceParametersReturns( - types.JSONObject{}, - ccv3.Warnings{"some-parameters-warning"}, - errors.New("not expected"), - ) - }) - - It("does not return an error, but returns warnings and the reason", func() { - Expect(executionError).NotTo(HaveOccurred()) - Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("not expected")) - }) - }) - }) - Describe("sharing", func() { When("targeting originating service instance space", func() { When("the service instance has shared spaces", func() { @@ -467,7 +411,7 @@ var _ = Describe("Service Instance Details Action", func() { It("returns an empty service instance, warnings, and the error", func() { Expect(serviceInstance).To(Equal(ServiceInstanceDetails{})) Expect(executionError).To(MatchError("error getting feature flag")) - Expect(warnings).To(ConsistOf("some-service-instance-warning", "some-parameters-warning", warningMessage)) + Expect(warnings).To(ConsistOf("some-service-instance-warning", warningMessage)) }) }) @@ -485,7 +429,7 @@ var _ = Describe("Service Instance Details Action", func() { It("returns an empty service instance, warnings, and the error", func() { Expect(serviceInstance).To(Equal(ServiceInstanceDetails{})) Expect(executionError).To(MatchError("error getting offering")) - Expect(warnings).To(ConsistOf("some-service-instance-warning", "some-parameters-warning", warningMessage)) + Expect(warnings).To(ConsistOf("some-service-instance-warning", warningMessage)) }) }) @@ -518,7 +462,7 @@ var _ = Describe("Service Instance Details Action", func() { It("returns an empty service instance, warnings, and the error", func() { Expect(serviceInstance).To(Equal(ServiceInstanceDetails{})) Expect(executionError).To(MatchError("no service instance")) - Expect(warnings).To(ConsistOf("some-service-instance-warning", "some-parameters-warning", warningMessage)) + Expect(warnings).To(ConsistOf("some-service-instance-warning", warningMessage)) }) }) @@ -542,7 +486,7 @@ var _ = Describe("Service Instance Details Action", func() { It("returns an empty service instance, warnings, and the error", func() { Expect(serviceInstance).To(Equal(ServiceInstanceDetails{})) Expect(executionError).To(MatchError("no service instance")) - Expect(warnings).To(ConsistOf("some-service-instance-warning", "some-parameters-warning", warningMessage)) + Expect(warnings).To(ConsistOf("some-service-instance-warning", warningMessage)) }) }) @@ -764,4 +708,148 @@ var _ = Describe("Service Instance Details Action", func() { }) }) }) + + FDescribe("GetServiceInstanceParameters", func() { + const ( + serviceInstanceName = "some-service-instance-name" + serviceInstanceGUID = "some-service-instance-guid" + spaceGUID = "some-source-space-guid" + ) + + var ( + serviceInstance ServiceInstanceDetails + warnings Warnings + executionError error + ) + + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceByNameAndSpaceReturns( + resources.ServiceInstance{ + Type: resources.ManagedServiceInstance, + Name: serviceInstanceName, + GUID: serviceInstanceGUID, + SpaceGUID: spaceGUID, + }, + ccv3.IncludedResources{}, + ccv3.Warnings{"some-service-instance-warning"}, + nil, + ) + + fakeCloudControllerClient.GetServiceInstanceParametersReturns( + types.JSONObject{"foo": "bar"}, + ccv3.Warnings{"some-parameters-warning"}, + nil, + ) + + }) + + JustBeforeEach(func() { + serviceInstance, warnings, executionError = actor.GetServiceInstanceParameters(serviceInstanceName, spaceGUID) + }) + + Describe("getting the service instance", func() { + It("makes the correct call to get the service instance", func() { + Expect(fakeCloudControllerClient.GetServiceInstanceByNameAndSpaceCallCount()).To(Equal(1)) + actualName, actualSpaceGUID, actualQuery := fakeCloudControllerClient.GetServiceInstanceByNameAndSpaceArgsForCall(0) + Expect(actualName).To(Equal(serviceInstanceName)) + Expect(actualSpaceGUID).To(Equal(spaceGUID)) + Expect(actualQuery).To(BeNil()) + }) + + When("the service instance cannot be found", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceByNameAndSpaceReturns( + resources.ServiceInstance{}, + ccv3.IncludedResources{}, + ccv3.Warnings{}, + ccerror.ServiceInstanceNotFoundError{Name: serviceInstanceName}, + ) + }) + + It("returns an error and warnings", func() { + Expect(executionError).To(MatchError(actionerror.ServiceInstanceNotFoundError{Name: serviceInstanceName})) + }) + + It("does not attempt any other requests", func() { + Expect(fakeCloudControllerClient.GetServiceInstanceParametersCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetFeatureFlagCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceOfferingByGUIDCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceInstanceSharedSpacesCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServicePlanByGUIDCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceCredentialBindingsCallCount()).To(Equal(0)) + }) + }) + + When("getting the service instance returns an error", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceByNameAndSpaceReturns( + resources.ServiceInstance{}, + ccv3.IncludedResources{}, + ccv3.Warnings{"some-service-instance-warning"}, + errors.New("no service instance")) + }) + + It("returns an error and warnings", func() { + Expect(executionError).To(MatchError("no service instance")) + Expect(warnings).To(ConsistOf("some-service-instance-warning")) + }) + + It("does not attempt any other requests", func() { + Expect(fakeCloudControllerClient.GetServiceInstanceParametersCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetFeatureFlagCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceOfferingByGUIDCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceInstanceSharedSpacesCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServicePlanByGUIDCallCount()).To(Equal(0)) + Expect(fakeCloudControllerClient.GetServiceCredentialBindingsCallCount()).To(Equal(0)) + }) + }) + }) + + Describe("getting the parameters", func() { + It("makes the correct call to get the parameters", func() { + Expect(fakeCloudControllerClient.GetServiceInstanceParametersCallCount()).To(Equal(1)) + Expect(fakeCloudControllerClient.GetServiceInstanceParametersArgsForCall(0)).To(Equal(serviceInstanceGUID)) + }) + + When("getting the parameters fails with an expected error", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceParametersReturns( + types.JSONObject{}, + ccv3.Warnings{"some-parameters-warning"}, + ccerror.V3UnexpectedResponseError{ + V3ErrorResponse: ccerror.V3ErrorResponse{ + Errors: []ccerror.V3Error{{ + Code: 1234, + Detail: "cannot get parameters reason", + Title: "CF-SomeRandomError", + }}, + }, + }, + ) + }) + + It("does not return an error, but returns warnings and the reason", func() { + Expect(executionError).NotTo(HaveOccurred()) + Expect(warnings).To(ContainElement("some-parameters-warning")) + Expect(serviceInstance.Parameters.MissingReason).To(Equal("cannot get parameters reason")) + }) + }) + + When("getting the parameters fails with an another error", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceParametersReturns( + types.JSONObject{}, + ccv3.Warnings{"some-parameters-warning"}, + errors.New("not expected"), + ) + }) + + It("does not return an error, but returns warnings and the reason", func() { + Expect(executionError).NotTo(HaveOccurred()) + Expect(warnings).To(ContainElement("some-parameters-warning")) + Expect(serviceInstance.Parameters.MissingReason).To(Equal("not expected")) + }) + }) + }) + }) }) diff --git a/command/v7/actor.go b/command/v7/actor.go index 2d4f388bc74..2f29be698a1 100644 --- a/command/v7/actor.go +++ b/command/v7/actor.go @@ -155,6 +155,7 @@ type Actor interface { GetServiceKeyDetailsByServiceInstanceAndName(serviceInstanceName, serviceKeyName, spaceGUID string) (resources.ServiceCredentialBindingDetails, v7action.Warnings, error) GetServiceInstanceByNameAndSpace(serviceInstanceName, spaceGUID string) (resources.ServiceInstance, v7action.Warnings, error) GetServiceInstanceDetails(serviceInstanceName, spaceGUID string, omitApps bool) (v7action.ServiceInstanceDetails, v7action.Warnings, error) + GetServiceInstanceParameters(serviceInstanceName, spaceGUID string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) GetServiceInstanceLabels(serviceInstanceName, spaceGUID string) (map[string]types.NullString, v7action.Warnings, error) GetServiceInstancesForSpace(spaceGUID string, omitApps bool) ([]v7action.ServiceInstance, v7action.Warnings, error) GetServiceKeysByServiceInstance(serviceInstanceName, spaceGUID string) ([]resources.ServiceCredentialBinding, v7action.Warnings, error) diff --git a/command/v7/service_command.go b/command/v7/service_command.go index 6ec4fc5f658..16177bd45ee 100644 --- a/command/v7/service_command.go +++ b/command/v7/service_command.go @@ -16,6 +16,7 @@ type ServiceCommand struct { RequiredArgs flag.ServiceInstance `positional-args:"yes"` ShowGUID bool `long:"guid" description:"Retrieve and display the given service instances's guid. All other output is suppressed."` + Params bool `long:"params" description:"Retrieve and display the given service instances's parameters. All other output is suppressed."` usage interface{} `usage:"CF_NAME service SERVICE_INSTANCE"` relatedCommands interface{} `related_commands:"bind-service, rename-service, update-service"` @@ -32,7 +33,12 @@ func (cmd ServiceCommand) Execute(args []string) error { err error ) - cmd.serviceInstance, warnings, err = cmd.Actor.GetServiceInstanceDetails(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID, false) + switch { + case cmd.Params: + cmd.serviceInstance, warnings, err = cmd.Actor.GetServiceInstanceParameters(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID) + default: + cmd.serviceInstance, warnings, err = cmd.Actor.GetServiceInstanceDetails(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID, false) + } cmd.UI.DisplayWarnings(warnings) if err != nil { return err @@ -41,6 +47,8 @@ func (cmd ServiceCommand) Execute(args []string) error { switch { case cmd.ShowGUID: return cmd.displayGUID() + case cmd.Params: + return cmd.displayParameters() case cmd.serviceInstance.Type == resources.UserProvidedServiceInstance: return cmd.chain( cmd.displayIntro, @@ -53,7 +61,6 @@ func (cmd ServiceCommand) Execute(args []string) error { cmd.displayPropertiesManaged, cmd.displayLastOperation, cmd.displayBoundApps, - cmd.displayParameters, cmd.displaySharingInfo, cmd.displayUpgrades, ) @@ -177,15 +184,7 @@ func (cmd ServiceCommand) displayParametersEmpty() { } func (cmd ServiceCommand) displayParametersData() { - cmd.UI.DisplayTextWithFlavor( - "Showing parameters for service instance {{.ServiceInstanceName}}...", - map[string]interface{}{ - "ServiceInstanceName": cmd.serviceInstance.Name, - }, - ) - cmd.UI.DisplayNewline() - - data, err := json.Marshal(cmd.serviceInstance.Parameters.Value) + data, err := json.MarshalIndent(cmd.serviceInstance.Parameters.Value, "", " ") if err != nil { panic(err) } diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go index 993c50ba271..5b7f2d7ff5c 100644 --- a/command/v7/service_command_test.go +++ b/command/v7/service_command_test.go @@ -256,9 +256,6 @@ var _ = Describe("service command", func() { }, ServicePlan: resources.ServicePlan{Name: servicePlanName}, ServiceBrokerName: serviceBrokerName, - Parameters: v7action.ServiceInstanceParameters{ - Value: types.JSONObject{"foo": "bar"}, - }, SharedStatus: v7action.SharedStatus{ IsSharedToOtherSpaces: true, UsageSummary: []v7action.UsageSummaryWithSpaceAndOrg{{"shared-to-space", "some-org", 3}}, @@ -302,10 +299,6 @@ var _ = Describe("service command", func() { Say(`Bound apps:\n`), Say(`There are no bound apps for this service instance\.\n`), Say(`\n`), - Say(`Showing parameters for service instance %s...\n`, serviceInstanceName), - Say(`\n`), - Say(`%s\n`, parameters), - Say(`\n`), Say(`Sharing:`), Say(`Shared with spaces:\n`), Say(`Upgrading:\n`), @@ -590,84 +583,6 @@ var _ = Describe("service command", func() { }) }) - Context("parameters", func() { - When("there was a problem retrieving the parameters", func() { - BeforeEach(func() { - fakeActor.GetServiceInstanceDetailsReturns( - v7action.ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - GUID: serviceInstanceGUID, - Name: serviceInstanceName, - Type: resources.ManagedServiceInstance, - LastOperation: resources.LastOperation{ - Type: lastOperationType, - State: lastOperationState, - Description: lastOperationDescription, - CreatedAt: lastOperationStartTime, - UpdatedAt: lastOperationUpdatedTime, - }, - }, - ServiceOffering: resources.ServiceOffering{ - Name: serviceOfferingName, - Description: serviceOfferingDescription, - DocumentationURL: serviceOfferingDocs, - }, - ServicePlan: resources.ServicePlan{Name: servicePlanName}, - ServiceBrokerName: serviceBrokerName, - Parameters: v7action.ServiceInstanceParameters{ - MissingReason: "because of a good reason", - }, - }, - v7action.Warnings{"warning one", "warning two"}, - nil, - ) - }) - - It("displays the reason", func() { - Expect(executeErr).NotTo(HaveOccurred()) - Expect(testUI.Out).To(Say(`Unable to show parameters: because of a good reason\n`)) - }) - }) - - When("the parameters are empty", func() { - BeforeEach(func() { - fakeActor.GetServiceInstanceDetailsReturns( - v7action.ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - GUID: serviceInstanceGUID, - Name: serviceInstanceName, - Type: resources.ManagedServiceInstance, - LastOperation: resources.LastOperation{ - Type: lastOperationType, - State: lastOperationState, - Description: lastOperationDescription, - CreatedAt: lastOperationStartTime, - UpdatedAt: lastOperationUpdatedTime, - }, - }, - ServiceOffering: resources.ServiceOffering{ - Name: serviceOfferingName, - Description: serviceOfferingDescription, - DocumentationURL: serviceOfferingDocs, - }, - ServicePlan: resources.ServicePlan{Name: servicePlanName}, - ServiceBrokerName: serviceBrokerName, - Parameters: v7action.ServiceInstanceParameters{ - Value: types.JSONObject{}, - }, - }, - v7action.Warnings{"warning one", "warning two"}, - nil, - ) - }) - - It("says there were no parameters", func() { - Expect(executeErr).NotTo(HaveOccurred()) - Expect(testUI.Out).To(Say(`No parameters are set for service instance %s.\n`, serviceInstanceName)) - }) - }) - }) - Context("bound apps", func() { BeforeEach(func() { fakeActor.GetServiceInstanceDetailsReturns( @@ -738,6 +653,102 @@ var _ = Describe("service command", func() { }) }) + When("the --params flag is specified", func() { + const ( + parameters = `{"foo":"bar"}` + ) + + BeforeEach(func() { + setFlag(&cmd, "--params") + }) + + When("parameters are set", func() { + BeforeEach(func() { + fakeActor.GetServiceInstanceParametersReturns( + v7action.ServiceInstanceDetails{ + ServiceInstance: resources.ServiceInstance{ + GUID: serviceInstanceGUID, + Name: serviceInstanceName, + }, + Parameters: v7action.ServiceInstanceParameters{ + Value: types.JSONObject{"foo": "bar"}, + }, + }, + v7action.Warnings{"warning one", "warning two"}, + nil, + ) + }) + + It("returns parameters JSON", func() { + Expect(executeErr).NotTo(HaveOccurred()) + + Expect(fakeActor.GetServiceInstanceParametersCallCount()).To(Equal(1)) + actualName, actualSpaceGUID := fakeActor.GetServiceInstanceParametersArgsForCall(0) + Expect(actualName).To(Equal(serviceInstanceName)) + Expect(actualSpaceGUID).To(Equal(spaceGUID)) + + Expect(testUI.Out).To(SatisfyAll( + Say(`\{\n`), + Say(` "foo": "bar"\n`), + Say(`\}\n`), + )) + Expect(testUI.Err).To(SatisfyAll( + Say("warning one"), + Say("warning two"), + )) + }) + }) + + When("there was a problem retrieving the parameters", func() { + BeforeEach(func() { + fakeActor.GetServiceInstanceParametersReturns( + v7action.ServiceInstanceDetails{ + ServiceInstance: resources.ServiceInstance{ + GUID: serviceInstanceGUID, + Name: serviceInstanceName, + Type: resources.ManagedServiceInstance, + }, + Parameters: v7action.ServiceInstanceParameters{ + MissingReason: "because of a good reason", + }, + }, + v7action.Warnings{"warning one", "warning two"}, + nil, + ) + }) + + It("displays the reason", func() { + Expect(executeErr).NotTo(HaveOccurred()) + Expect(testUI.Out).To(Say(`Unable to show parameters: because of a good reason\n`)) + }) + }) + + When("the parameters are empty", func() { + BeforeEach(func() { + fakeActor.GetServiceInstanceParametersReturns( + v7action.ServiceInstanceDetails{ + ServiceInstance: resources.ServiceInstance{ + GUID: serviceInstanceGUID, + Name: serviceInstanceName, + Type: resources.ManagedServiceInstance, + }, + Parameters: v7action.ServiceInstanceParameters{ + Value: types.JSONObject{}, + }, + }, + v7action.Warnings{"warning one", "warning two"}, + nil, + ) + }) + + It("says there were no parameters", func() { + Expect(executeErr).NotTo(HaveOccurred()) + Expect(testUI.Out).To(Say(`No parameters are set for service instance %s.\n`, serviceInstanceName)) + }) + }) + + }) + When("there is a problem looking up the service instance", func() { BeforeEach(func() { fakeActor.GetServiceInstanceDetailsReturns( diff --git a/command/v7/v7fakes/fake_actor.go b/command/v7/v7fakes/fake_actor.go index 444dc2395ae..7a5067cc813 100644 --- a/command/v7/v7fakes/fake_actor.go +++ b/command/v7/v7fakes/fake_actor.go @@ -2046,6 +2046,22 @@ type FakeActor struct { result2 v7action.Warnings result3 error } + GetServiceInstanceParametersStub func(string, string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) + getServiceInstanceParametersMutex sync.RWMutex + getServiceInstanceParametersArgsForCall []struct { + arg1 string + arg2 string + } + getServiceInstanceParametersReturns struct { + result1 v7action.ServiceInstanceDetails + result2 v7action.Warnings + result3 error + } + getServiceInstanceParametersReturnsOnCall map[int]struct { + result1 v7action.ServiceInstanceDetails + result2 v7action.Warnings + result3 error + } GetServiceInstancesForSpaceStub func(string, bool) ([]v7action.ServiceInstance, v7action.Warnings, error) getServiceInstancesForSpaceMutex sync.RWMutex getServiceInstancesForSpaceArgsForCall []struct { @@ -12247,6 +12263,73 @@ func (fake *FakeActor) GetServiceInstanceLabelsReturnsOnCall(i int, result1 map[ }{result1, result2, result3} } +func (fake *FakeActor) GetServiceInstanceParameters(arg1 string, arg2 string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) { + fake.getServiceInstanceParametersMutex.Lock() + ret, specificReturn := fake.getServiceInstanceParametersReturnsOnCall[len(fake.getServiceInstanceParametersArgsForCall)] + fake.getServiceInstanceParametersArgsForCall = append(fake.getServiceInstanceParametersArgsForCall, struct { + arg1 string + arg2 string + }{arg1, arg2}) + fake.recordInvocation("GetServiceInstanceParameters", []interface{}{arg1, arg2}) + fake.getServiceInstanceParametersMutex.Unlock() + if fake.GetServiceInstanceParametersStub != nil { + return fake.GetServiceInstanceParametersStub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + fakeReturns := fake.getServiceInstanceParametersReturns + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 +} + +func (fake *FakeActor) GetServiceInstanceParametersCallCount() int { + fake.getServiceInstanceParametersMutex.RLock() + defer fake.getServiceInstanceParametersMutex.RUnlock() + return len(fake.getServiceInstanceParametersArgsForCall) +} + +func (fake *FakeActor) GetServiceInstanceParametersCalls(stub func(string, string) (v7action.ServiceInstanceDetails, v7action.Warnings, error)) { + fake.getServiceInstanceParametersMutex.Lock() + defer fake.getServiceInstanceParametersMutex.Unlock() + fake.GetServiceInstanceParametersStub = stub +} + +func (fake *FakeActor) GetServiceInstanceParametersArgsForCall(i int) (string, string) { + fake.getServiceInstanceParametersMutex.RLock() + defer fake.getServiceInstanceParametersMutex.RUnlock() + argsForCall := fake.getServiceInstanceParametersArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeActor) GetServiceInstanceParametersReturns(result1 v7action.ServiceInstanceDetails, result2 v7action.Warnings, result3 error) { + fake.getServiceInstanceParametersMutex.Lock() + defer fake.getServiceInstanceParametersMutex.Unlock() + fake.GetServiceInstanceParametersStub = nil + fake.getServiceInstanceParametersReturns = struct { + result1 v7action.ServiceInstanceDetails + result2 v7action.Warnings + result3 error + }{result1, result2, result3} +} + +func (fake *FakeActor) GetServiceInstanceParametersReturnsOnCall(i int, result1 v7action.ServiceInstanceDetails, result2 v7action.Warnings, result3 error) { + fake.getServiceInstanceParametersMutex.Lock() + defer fake.getServiceInstanceParametersMutex.Unlock() + fake.GetServiceInstanceParametersStub = nil + if fake.getServiceInstanceParametersReturnsOnCall == nil { + fake.getServiceInstanceParametersReturnsOnCall = make(map[int]struct { + result1 v7action.ServiceInstanceDetails + result2 v7action.Warnings + result3 error + }) + } + fake.getServiceInstanceParametersReturnsOnCall[i] = struct { + result1 v7action.ServiceInstanceDetails + result2 v7action.Warnings + result3 error + }{result1, result2, result3} +} + func (fake *FakeActor) GetServiceInstancesForSpace(arg1 string, arg2 bool) ([]v7action.ServiceInstance, v7action.Warnings, error) { fake.getServiceInstancesForSpaceMutex.Lock() ret, specificReturn := fake.getServiceInstancesForSpaceReturnsOnCall[len(fake.getServiceInstancesForSpaceArgsForCall)] @@ -18878,6 +18961,8 @@ func (fake *FakeActor) Invocations() map[string][][]interface{} { defer fake.getServiceInstanceDetailsMutex.RUnlock() fake.getServiceInstanceLabelsMutex.RLock() defer fake.getServiceInstanceLabelsMutex.RUnlock() + fake.getServiceInstanceParametersMutex.RLock() + defer fake.getServiceInstanceParametersMutex.RUnlock() fake.getServiceInstancesForSpaceMutex.RLock() defer fake.getServiceInstancesForSpaceMutex.RUnlock() fake.getServiceKeyByServiceInstanceAndNameMutex.RLock() diff --git a/integration/v7/isolated/service_command_test.go b/integration/v7/isolated/service_command_test.go index 49fcaf2717b..ed01afd0d45 100644 --- a/integration/v7/isolated/service_command_test.go +++ b/integration/v7/isolated/service_command_test.go @@ -28,6 +28,7 @@ var _ = Describe("service command", func() { Say(`\n`), Say(`OPTIONS:\n`), Say(`\s+--guid\s+Retrieve and display the given service instances's guid. All other output is suppressed.\n`), + Say(`\s+--params\s+Retrieve and display the given service instances's parameters. All other output is suppressed.\n`), Say(`\n`), Say(`SEE ALSO:\n`), Say(`\s+bind-service, rename-service, update-service\n`), @@ -233,8 +234,6 @@ var _ = Describe("service command", func() { Say(`Bound apps:\n`), Say(`There are no bound apps for this service instance\.\n`), Say(`\n`), - Say(`No parameters are set for service instance %s.\n`, serviceInstanceName), - Say(`\n`), Say(`Sharing:\n`), Say(`This service instance is not currently being shared.`), Say(`\n`), @@ -297,8 +296,6 @@ var _ = Describe("service command", func() { Say(`Bound apps:\n`), Say(`There are no bound apps for this service instance\.\n`), Say(`\n`), - Say(`Unable to show parameters: An operation for service instance %s is in progress.`, serviceInstanceName), - Say(`\n`), Say(`Sharing:\n`), Say(`This service instance is not currently being shared.`), Say(`\n`), @@ -308,36 +305,6 @@ var _ = Describe("service command", func() { }) }) - When("service instance parameters have been set", func() { - var parameters string - - BeforeEach(func() { - broker = servicebrokerstub.EnableServiceAccess() - parameters = fmt.Sprintf(`{"foo":"%s"}`, helpers.RandomName()) - command := []string{ - "create-service", - broker.FirstServiceOfferingName(), - broker.FirstServicePlanName(), - serviceInstanceName, - "-c", parameters, - } - Eventually(helpers.CF(command...)).Should(Exit(0)) - Eventually(helpers.CF(serviceCommand, serviceInstanceName)).Should(Say(`status:\s+create succeeded`)) - }) - - It("reports the service instance parameters", func() { - session := helpers.CF(serviceCommand, serviceInstanceName) - Eventually(session).Should(Exit(0)) - - Expect(session).To(SatisfyAll( - Say(`Showing parameters for service instance %s...\n`, serviceInstanceName), - Say(`\n`), - Say(`%s\n`, parameters), - Say(`\n`), - )) - }) - }) - When("the instance is shared with another space", func() { var sharedToSpaceName string @@ -548,5 +515,44 @@ var _ = Describe("service command", func() { }) }) }) + + When("--params is requested", func() { + var broker *servicebrokerstub.ServiceBrokerStub + + AfterEach(func() { + broker.Forget() + }) + + When("service instance parameters have been set", func() { + var key string + var value string + + BeforeEach(func() { + broker = servicebrokerstub.EnableServiceAccess() + key = "foo" + value = helpers.RandomName() + command := []string{ + "create-service", + broker.FirstServiceOfferingName(), + broker.FirstServicePlanName(), + serviceInstanceName, + "-c", fmt.Sprintf(`{"%s":"%s"}`, key, value), + } + Eventually(helpers.CF(command...)).Should(Exit(0)) + Eventually(helpers.CF(serviceCommand, serviceInstanceName)).Should(Say(`status:\s+create succeeded`)) + }) + + FIt("reports the service instance parameters JSON", func() { + session := helpers.CF(serviceCommand, serviceInstanceName, "--params") + Eventually(session).Should(Exit(0)) + + Expect(session).To(SatisfyAll( + Say(`\{\n`), + Say(` %q: %q\n`, key, value), + Say(`\}\n`), + )) + }) + }) + }) }) }) From 7061d71a9d9388d46b0cc16e70ab20aca19905c3 Mon Sep 17 00:00:00 2001 From: Felisia Martini <fmartini@pivotal.io> Date: Thu, 18 Feb 2021 10:40:28 +0000 Subject: [PATCH 2/6] Added ServiceInstanceParametersFetchNotSupportedError --- actor/v7action/service_instance_details.go | 20 ++++++---- .../v7action/service_instance_details_test.go | 38 ++++++++++++++++++- ...ce_parameters_fetch_not_supported_error.go | 12 ++++++ api/cloudcontroller/ccv3/errors.go | 3 +- api/cloudcontroller/ccv3/errors_test.go | 20 ++++++++++ command/v7/service_command_test.go | 1 - 6 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 api/cloudcontroller/ccerror/service_instance_parameters_fetch_not_supported_error.go diff --git a/actor/v7action/service_instance_details.go b/actor/v7action/service_instance_details.go index 355696f294a..83f678ce67a 100644 --- a/actor/v7action/service_instance_details.go +++ b/actor/v7action/service_instance_details.go @@ -160,15 +160,19 @@ func (actor Actor) getServiceInstanceDetails(serviceInstanceName string, spaceGU func (actor Actor) getServiceInstanceParameters(serviceInstanceGUID string) (ServiceInstanceParameters, ccv3.Warnings) { params, warnings, err := actor.CloudControllerClient.GetServiceInstanceParameters(serviceInstanceGUID) - if err != nil { - if e, ok := err.(ccerror.V3UnexpectedResponseError); ok && len(e.Errors) > 0 { - return ServiceInstanceParameters{MissingReason: e.Errors[0].Detail}, warnings - } else { - return ServiceInstanceParameters{MissingReason: err.Error()}, warnings - } - } - return ServiceInstanceParameters{Value: params}, warnings + switch err := err.(type) { + case nil: + return ServiceInstanceParameters{Value: params}, warnings + case ccerror.ResourceNotFoundError: // Old API versions did not produce a helpful error + return ServiceInstanceParameters{MissingReason: "This service does not support fetching service instance parameters."}, warnings + case ccerror.ServiceInstanceParametersFetchNotSupportedError: + return ServiceInstanceParameters{MissingReason: err.Message}, warnings + case ccerror.V3UnexpectedResponseError: + return ServiceInstanceParameters{MissingReason: err.Errors[0].Detail}, warnings + default: + return ServiceInstanceParameters{MissingReason: err.Error()}, warnings + } } func (actor Actor) getServiceInstanceSharedStatus(serviceInstanceDetails ServiceInstanceDetails, targetedSpace string) (SharedStatus, ccv3.Warnings, error) { diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go index 24c09901c0e..c643c53d752 100644 --- a/actor/v7action/service_instance_details_test.go +++ b/actor/v7action/service_instance_details_test.go @@ -811,7 +811,7 @@ var _ = Describe("Service Instance Details Action", func() { Expect(fakeCloudControllerClient.GetServiceInstanceParametersArgsForCall(0)).To(Equal(serviceInstanceGUID)) }) - When("getting the parameters fails with an expected error", func() { + When("getting the parameters fails with a V3UnexpectedResponseError", func() { BeforeEach(func() { fakeCloudControllerClient.GetServiceInstanceParametersReturns( types.JSONObject{}, @@ -835,6 +835,42 @@ var _ = Describe("Service Instance Details Action", func() { }) }) + When("getting the parameters fails with a ServiceInstanceParametersFetchNotSupportedError", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceParametersReturns( + types.JSONObject{}, + ccv3.Warnings{"some-parameters-warning"}, + ccerror.ServiceInstanceParametersFetchNotSupportedError{ + Message: "This service does not support fetching service instance parameters.", + }, + ) + }) + + It("does not return an error, but returns warnings and the reason", func() { + Expect(executionError).NotTo(HaveOccurred()) + Expect(warnings).To(ContainElement("some-parameters-warning")) + Expect(serviceInstance.Parameters.MissingReason).To(Equal("This service does not support fetching service instance parameters.")) + }) + }) + + When("getting the parameters fails with a ResourceNotFoundError", func() { + BeforeEach(func() { + fakeCloudControllerClient.GetServiceInstanceParametersReturns( + types.JSONObject{}, + ccv3.Warnings{"some-parameters-warning"}, + ccerror.ResourceNotFoundError{ + Message: "Service instance not found", + }, + ) + }) + + It("converts the error to fetching not supported", func() { + Expect(executionError).NotTo(HaveOccurred()) + Expect(warnings).To(ContainElement("some-parameters-warning")) + Expect(serviceInstance.Parameters.MissingReason).To(Equal("This service does not support fetching service instance parameters.")) + }) + }) + When("getting the parameters fails with an another error", func() { BeforeEach(func() { fakeCloudControllerClient.GetServiceInstanceParametersReturns( diff --git a/api/cloudcontroller/ccerror/service_instance_parameters_fetch_not_supported_error.go b/api/cloudcontroller/ccerror/service_instance_parameters_fetch_not_supported_error.go new file mode 100644 index 00000000000..51ed9f56886 --- /dev/null +++ b/api/cloudcontroller/ccerror/service_instance_parameters_fetch_not_supported_error.go @@ -0,0 +1,12 @@ +package ccerror + +// ServiceInstanceParametersFetchNotSupportedError is returned when +// the service instance is user-provided or +// service instance fetching is not supported for managed instances +type ServiceInstanceParametersFetchNotSupportedError struct { + Message string +} + +func (e ServiceInstanceParametersFetchNotSupportedError) Error() string { + return e.Message +} diff --git a/api/cloudcontroller/ccv3/errors.go b/api/cloudcontroller/ccv3/errors.go index 511c828520c..1a593a5fe1f 100644 --- a/api/cloudcontroller/ccv3/errors.go +++ b/api/cloudcontroller/ccv3/errors.go @@ -115,9 +115,10 @@ func handleBadRequest(errorResponse ccerror.V3Error, _ *cloudcontroller.Request) switch errorResponse.Detail { case "Bad request: Cannot stage package whose state is not ready.": return ccerror.InvalidStateError{} + case "This service does not support fetching service instance parameters.": + return ccerror.ServiceInstanceParametersFetchNotSupportedError{Message: errorResponse.Detail} default: return ccerror.BadRequestError{Message: errorResponse.Detail} - } } diff --git a/api/cloudcontroller/ccv3/errors_test.go b/api/cloudcontroller/ccv3/errors_test.go index f55b925c9d0..3e6c8e0b3bc 100644 --- a/api/cloudcontroller/ccv3/errors_test.go +++ b/api/cloudcontroller/ccv3/errors_test.go @@ -107,6 +107,26 @@ var _ = Describe("Error Wrapper", func() { }) }) + + When("service instance fetch params not supported", func() { + BeforeEach(func() { + serverResponse = ` +{ + "errors": [ + { + "detail": "This service does not support fetching service instance parameters.", + "title": "CF-ServiceFetchInstanceParametersNotSupported", + "code": 120004 + } + ] +}` + }) + + It("returns a BadRequestError", func() { + Expect(makeError).To(MatchError(ccerror.ServiceInstanceParametersFetchNotSupportedError{ + Message: "This service does not support fetching service instance parameters."})) + }) + }) }) Context("(401) Unauthorized", func() { diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go index 5b7f2d7ff5c..d99f5fa5a6d 100644 --- a/command/v7/service_command_test.go +++ b/command/v7/service_command_test.go @@ -746,7 +746,6 @@ var _ = Describe("service command", func() { Expect(testUI.Out).To(Say(`No parameters are set for service instance %s.\n`, serviceInstanceName)) }) }) - }) When("there is a problem looking up the service instance", func() { From d91ea43751be59178251a3321e12db3f83c4e352 Mon Sep 17 00:00:00 2001 From: Felisia Martini <fmartini@pivotal.io> Date: Thu, 18 Feb 2021 10:41:11 +0000 Subject: [PATCH 3/6] Remove test focus --- actor/v7action/service_instance_details_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go index c643c53d752..f5bde68f62d 100644 --- a/actor/v7action/service_instance_details_test.go +++ b/actor/v7action/service_instance_details_test.go @@ -709,7 +709,7 @@ var _ = Describe("Service Instance Details Action", func() { }) }) - FDescribe("GetServiceInstanceParameters", func() { + Describe("GetServiceInstanceParameters", func() { const ( serviceInstanceName = "some-service-instance-name" serviceInstanceGUID = "some-service-instance-guid" From fb7b3e433b7684699214019db5b2cc85179d4808 Mon Sep 17 00:00:00 2001 From: Felisia Martini <fmartini@pivotal.io> Date: Thu, 18 Feb 2021 11:52:52 +0000 Subject: [PATCH 4/6] Error when params not retriavable --- ...nce_params_fetching_not_supported_error.go | 11 +++++ actor/v7action/service_instance_details.go | 21 ++++----- .../v7action/service_instance_details_test.go | 44 +++++++------------ command/v7/service_command.go | 11 ----- command/v7/service_command_test.go | 13 +++--- .../v7/isolated/service_command_test.go | 34 +++++++------- 6 files changed, 62 insertions(+), 72 deletions(-) create mode 100644 actor/actionerror/service_instance_params_fetching_not_supported_error.go diff --git a/actor/actionerror/service_instance_params_fetching_not_supported_error.go b/actor/actionerror/service_instance_params_fetching_not_supported_error.go new file mode 100644 index 00000000000..bf15682d8d0 --- /dev/null +++ b/actor/actionerror/service_instance_params_fetching_not_supported_error.go @@ -0,0 +1,11 @@ +package actionerror + +// ServiceInstanceParamsFetchingNotSupportedError is returned when +// service instance is user provided or +// service instance retrievable is false for a managed service instance +type ServiceInstanceParamsFetchingNotSupportedError struct { +} + +func (e ServiceInstanceParamsFetchingNotSupportedError) Error() string { + return "This service does not support fetching service instance parameters." +} diff --git a/actor/v7action/service_instance_details.go b/actor/v7action/service_instance_details.go index 83f678ce67a..337e80ee45d 100644 --- a/actor/v7action/service_instance_details.go +++ b/actor/v7action/service_instance_details.go @@ -33,8 +33,7 @@ type SharedStatus struct { } type ServiceInstanceParameters struct { - Value types.JSONObject - MissingReason string + Value types.JSONObject } type ServiceInstanceUpgradeState int @@ -102,7 +101,7 @@ func (actor Actor) GetServiceInstanceParameters(serviceInstanceName string, spac return }, func() (warnings ccv3.Warnings, err error) { - serviceInstanceDetails.Parameters, warnings = actor.getServiceInstanceParameters(serviceInstanceDetails.ServiceInstance.GUID) + serviceInstanceDetails.Parameters, warnings, err = actor.getServiceInstanceParameters(serviceInstanceDetails.ServiceInstance.GUID) return }, ) @@ -158,21 +157,19 @@ func (actor Actor) getServiceInstanceDetails(serviceInstanceName string, spaceGU return result, warnings, nil } -func (actor Actor) getServiceInstanceParameters(serviceInstanceGUID string) (ServiceInstanceParameters, ccv3.Warnings) { +func (actor Actor) getServiceInstanceParameters(serviceInstanceGUID string) (ServiceInstanceParameters, ccv3.Warnings, error) { params, warnings, err := actor.CloudControllerClient.GetServiceInstanceParameters(serviceInstanceGUID) switch err := err.(type) { case nil: - return ServiceInstanceParameters{Value: params}, warnings - case ccerror.ResourceNotFoundError: // Old API versions did not produce a helpful error - return ServiceInstanceParameters{MissingReason: "This service does not support fetching service instance parameters."}, warnings - case ccerror.ServiceInstanceParametersFetchNotSupportedError: - return ServiceInstanceParameters{MissingReason: err.Message}, warnings - case ccerror.V3UnexpectedResponseError: - return ServiceInstanceParameters{MissingReason: err.Errors[0].Detail}, warnings + case ccerror.ResourceNotFoundError, + ccerror.ServiceInstanceParametersFetchNotSupportedError: + return ServiceInstanceParameters{}, warnings, actionerror.ServiceInstanceParamsFetchingNotSupportedError{} default: - return ServiceInstanceParameters{MissingReason: err.Error()}, warnings + return ServiceInstanceParameters{}, warnings, err } + + return ServiceInstanceParameters{Value: params}, warnings, nil } func (actor Actor) getServiceInstanceSharedStatus(serviceInstanceDetails ServiceInstanceDetails, targetedSpace string) (SharedStatus, ccv3.Warnings, error) { diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go index f5bde68f62d..496ff736140 100644 --- a/actor/v7action/service_instance_details_test.go +++ b/actor/v7action/service_instance_details_test.go @@ -811,28 +811,21 @@ var _ = Describe("Service Instance Details Action", func() { Expect(fakeCloudControllerClient.GetServiceInstanceParametersArgsForCall(0)).To(Equal(serviceInstanceGUID)) }) - When("getting the parameters fails with a V3UnexpectedResponseError", func() { - BeforeEach(func() { - fakeCloudControllerClient.GetServiceInstanceParametersReturns( - types.JSONObject{}, - ccv3.Warnings{"some-parameters-warning"}, - ccerror.V3UnexpectedResponseError{ - V3ErrorResponse: ccerror.V3ErrorResponse{ - Errors: []ccerror.V3Error{{ - Code: 1234, - Detail: "cannot get parameters reason", - Title: "CF-SomeRandomError", - }}, - }, + It("returns parameters", func() { + Expect(serviceInstance).To(Equal( + ServiceInstanceDetails{ + ServiceInstance: resources.ServiceInstance{ + Type: resources.ManagedServiceInstance, + Name: serviceInstanceName, + GUID: serviceInstanceGUID, + SpaceGUID: spaceGUID, }, - ) - }) - - It("does not return an error, but returns warnings and the reason", func() { - Expect(executionError).NotTo(HaveOccurred()) - Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("cannot get parameters reason")) - }) + SharedStatus: SharedStatus{}, + Parameters: ServiceInstanceParameters{ + Value: types.JSONObject{"foo": "bar"}, + }, + }, + )) }) When("getting the parameters fails with a ServiceInstanceParametersFetchNotSupportedError", func() { @@ -847,9 +840,8 @@ var _ = Describe("Service Instance Details Action", func() { }) It("does not return an error, but returns warnings and the reason", func() { - Expect(executionError).NotTo(HaveOccurred()) + Expect(executionError).To(MatchError(actionerror.ServiceInstanceParamsFetchingNotSupportedError{})) Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("This service does not support fetching service instance parameters.")) }) }) @@ -865,9 +857,8 @@ var _ = Describe("Service Instance Details Action", func() { }) It("converts the error to fetching not supported", func() { - Expect(executionError).NotTo(HaveOccurred()) + Expect(executionError).To(MatchError(actionerror.ServiceInstanceParamsFetchingNotSupportedError{})) Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("This service does not support fetching service instance parameters.")) }) }) @@ -881,9 +872,8 @@ var _ = Describe("Service Instance Details Action", func() { }) It("does not return an error, but returns warnings and the reason", func() { - Expect(executionError).NotTo(HaveOccurred()) + Expect(executionError).To(MatchError("not expected")) Expect(warnings).To(ContainElement("some-parameters-warning")) - Expect(serviceInstance.Parameters.MissingReason).To(Equal("not expected")) }) }) }) diff --git a/command/v7/service_command.go b/command/v7/service_command.go index 16177bd45ee..e492a626d07 100644 --- a/command/v7/service_command.go +++ b/command/v7/service_command.go @@ -163,8 +163,6 @@ func (cmd ServiceCommand) displayLastOperation() error { func (cmd ServiceCommand) displayParameters() error { switch { - case cmd.serviceInstance.Parameters.MissingReason != "": - cmd.displayParametersMissingReason() case len(cmd.serviceInstance.Parameters.Value) > 0: cmd.displayParametersData() default: @@ -192,15 +190,6 @@ func (cmd ServiceCommand) displayParametersData() { cmd.UI.DisplayText(string(data)) } -func (cmd ServiceCommand) displayParametersMissingReason() { - cmd.UI.DisplayText( - "Unable to show parameters: {{.Reason}}", - map[string]interface{}{ - "Reason": cmd.serviceInstance.Parameters.MissingReason, - }, - ) -} - func (cmd ServiceCommand) displayIntro() error { user, err := cmd.Config.CurrentUser() if err != nil { diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go index d99f5fa5a6d..89ba7ef323c 100644 --- a/command/v7/service_command_test.go +++ b/command/v7/service_command_test.go @@ -708,18 +708,19 @@ var _ = Describe("service command", func() { Name: serviceInstanceName, Type: resources.ManagedServiceInstance, }, - Parameters: v7action.ServiceInstanceParameters{ - MissingReason: "because of a good reason", - }, + Parameters: v7action.ServiceInstanceParameters{}, }, v7action.Warnings{"warning one", "warning two"}, - nil, + errors.New("something happened"), ) }) It("displays the reason", func() { - Expect(executeErr).NotTo(HaveOccurred()) - Expect(testUI.Out).To(Say(`Unable to show parameters: because of a good reason\n`)) + Expect(executeErr).To(MatchError("something happened")) + Expect(testUI.Err).To(SatisfyAll( + Say("warning one"), + Say("warning two"), + )) }) }) diff --git a/integration/v7/isolated/service_command_test.go b/integration/v7/isolated/service_command_test.go index ed01afd0d45..04e1e027fab 100644 --- a/integration/v7/isolated/service_command_test.go +++ b/integration/v7/isolated/service_command_test.go @@ -176,6 +176,18 @@ var _ = Describe("service command", func() { )) }) }) + + When("--params is requested", func() { + When("service instance parameters have been set", func() { + It("reports the service instance parameters JSON", func() { + session := helpers.CF(serviceCommand, serviceInstanceName, "--params") + Eventually(session).Should(Exit(1)) + + Eventually(session.Err).Should(Say("This service does not support fetching service instance parameters.")) + }) + }) + }) + }) When("the service instance is managed by a broker", func() { @@ -514,35 +526,25 @@ var _ = Describe("service command", func() { )) }) }) - }) - - When("--params is requested", func() { - var broker *servicebrokerstub.ServiceBrokerStub - AfterEach(func() { - broker.Forget() - }) - - When("service instance parameters have been set", func() { + When("--params is requested", func() { var key string var value string BeforeEach(func() { - broker = servicebrokerstub.EnableServiceAccess() key = "foo" value = helpers.RandomName() - command := []string{ - "create-service", + + broker = servicebrokerstub.New().EnableServiceAccess() + helpers.CreateManagedServiceInstance( broker.FirstServiceOfferingName(), broker.FirstServicePlanName(), serviceInstanceName, "-c", fmt.Sprintf(`{"%s":"%s"}`, key, value), - } - Eventually(helpers.CF(command...)).Should(Exit(0)) - Eventually(helpers.CF(serviceCommand, serviceInstanceName)).Should(Say(`status:\s+create succeeded`)) + ) }) - FIt("reports the service instance parameters JSON", func() { + It("reports the service instance parameters JSON", func() { session := helpers.CF(serviceCommand, serviceInstanceName, "--params") Eventually(session).Should(Exit(0)) From 0e43f1761c3ed0eebddd65e2f331aaa48ceb6f55 Mon Sep 17 00:00:00 2001 From: Felisia Martini <fmartini@pivotal.io> Date: Thu, 18 Feb 2021 15:25:02 +0000 Subject: [PATCH 5/6] Address PR feedback --- actor/v7action/service_instance_details.go | 21 +++-- .../v7action/service_instance_details_test.go | 25 ++---- api/cloudcontroller/ccv3/errors_test.go | 2 +- command/v7/actor.go | 2 +- command/v7/service_command.go | 78 ++++++++++++------- command/v7/service_command_test.go | 39 +--------- command/v7/v7fakes/fake_actor.go | 20 ++--- .../v7/isolated/service_command_test.go | 2 +- 8 files changed, 82 insertions(+), 107 deletions(-) diff --git a/actor/v7action/service_instance_details.go b/actor/v7action/service_instance_details.go index 337e80ee45d..a12838cb3b5 100644 --- a/actor/v7action/service_instance_details.go +++ b/actor/v7action/service_instance_details.go @@ -32,9 +32,7 @@ type SharedStatus struct { UsageSummary []UsageSummaryWithSpaceAndOrg } -type ServiceInstanceParameters struct { - Value types.JSONObject -} +type ServiceInstanceParameters types.JSONObject type ServiceInstanceUpgradeState int @@ -56,7 +54,6 @@ type ServiceInstanceDetails struct { ServiceOffering resources.ServiceOffering ServicePlan resources.ServicePlan ServiceBrokerName string - Parameters ServiceInstanceParameters SharedStatus SharedStatus UpgradeStatus ServiceInstanceUpgradeStatus BoundApps []resources.ServiceCredentialBinding @@ -92,24 +89,25 @@ func (actor Actor) GetServiceInstanceDetails(serviceInstanceName string, spaceGU return serviceInstanceDetails, Warnings(warnings), nil } -func (actor Actor) GetServiceInstanceParameters(serviceInstanceName string, spaceGUID string) (ServiceInstanceDetails, Warnings, error) { - var serviceInstanceDetails ServiceInstanceDetails +func (actor Actor) GetServiceInstanceParameters(serviceInstanceName string, spaceGUID string) (ServiceInstanceParameters, Warnings, error) { + var serviceInstance resources.ServiceInstance + var parameters ServiceInstanceParameters warnings, err := railway.Sequentially( func() (warnings ccv3.Warnings, err error) { - serviceInstanceDetails.ServiceInstance, _, warnings, err = actor.getServiceInstanceByNameAndSpace(serviceInstanceName, spaceGUID) + serviceInstance, _, warnings, err = actor.getServiceInstanceByNameAndSpace(serviceInstanceName, spaceGUID) return }, func() (warnings ccv3.Warnings, err error) { - serviceInstanceDetails.Parameters, warnings, err = actor.getServiceInstanceParameters(serviceInstanceDetails.ServiceInstance.GUID) + parameters, warnings, err = actor.getServiceInstanceParameters(serviceInstance.GUID) return }, ) if err != nil { - return ServiceInstanceDetails{}, Warnings(warnings), err + return ServiceInstanceParameters{}, Warnings(warnings), err } - return serviceInstanceDetails, Warnings(warnings), nil + return parameters, Warnings(warnings), nil } func (actor Actor) getServiceInstanceDetails(serviceInstanceName string, spaceGUID string) (ServiceInstanceDetails, ccv3.Warnings, error) { @@ -162,14 +160,13 @@ func (actor Actor) getServiceInstanceParameters(serviceInstanceGUID string) (Ser switch err := err.(type) { case nil: + return ServiceInstanceParameters(params), warnings, nil case ccerror.ResourceNotFoundError, ccerror.ServiceInstanceParametersFetchNotSupportedError: return ServiceInstanceParameters{}, warnings, actionerror.ServiceInstanceParamsFetchingNotSupportedError{} default: return ServiceInstanceParameters{}, warnings, err } - - return ServiceInstanceParameters{Value: params}, warnings, nil } func (actor Actor) getServiceInstanceSharedStatus(serviceInstanceDetails ServiceInstanceDetails, targetedSpace string) (SharedStatus, ccv3.Warnings, error) { diff --git a/actor/v7action/service_instance_details_test.go b/actor/v7action/service_instance_details_test.go index 496ff736140..8f4ad0adbdf 100644 --- a/actor/v7action/service_instance_details_test.go +++ b/actor/v7action/service_instance_details_test.go @@ -148,7 +148,6 @@ var _ = Describe("Service Instance Details Action", func() { ServicePlan: resources.ServicePlan{Name: servicePlanName}, ServiceBrokerName: serviceBrokerName, SharedStatus: SharedStatus{}, - Parameters: ServiceInstanceParameters{}, BoundApps: []resources.ServiceCredentialBinding{ { Type: resources.AppBinding, @@ -717,9 +716,9 @@ var _ = Describe("Service Instance Details Action", func() { ) var ( - serviceInstance ServiceInstanceDetails - warnings Warnings - executionError error + serviceInstanceParams ServiceInstanceParameters + warnings Warnings + executionError error ) BeforeEach(func() { @@ -744,7 +743,7 @@ var _ = Describe("Service Instance Details Action", func() { }) JustBeforeEach(func() { - serviceInstance, warnings, executionError = actor.GetServiceInstanceParameters(serviceInstanceName, spaceGUID) + serviceInstanceParams, warnings, executionError = actor.GetServiceInstanceParameters(serviceInstanceName, spaceGUID) }) Describe("getting the service instance", func() { @@ -812,20 +811,8 @@ var _ = Describe("Service Instance Details Action", func() { }) It("returns parameters", func() { - Expect(serviceInstance).To(Equal( - ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - Type: resources.ManagedServiceInstance, - Name: serviceInstanceName, - GUID: serviceInstanceGUID, - SpaceGUID: spaceGUID, - }, - SharedStatus: SharedStatus{}, - Parameters: ServiceInstanceParameters{ - Value: types.JSONObject{"foo": "bar"}, - }, - }, - )) + Expect(serviceInstanceParams).To(Equal( + ServiceInstanceParameters(types.JSONObject{"foo": "bar"}))) }) When("getting the parameters fails with a ServiceInstanceParametersFetchNotSupportedError", func() { diff --git a/api/cloudcontroller/ccv3/errors_test.go b/api/cloudcontroller/ccv3/errors_test.go index 3e6c8e0b3bc..8785a9a848c 100644 --- a/api/cloudcontroller/ccv3/errors_test.go +++ b/api/cloudcontroller/ccv3/errors_test.go @@ -122,7 +122,7 @@ var _ = Describe("Error Wrapper", func() { }` }) - It("returns a BadRequestError", func() { + It("returns a ServiceInstanceParametersFetchNotSupportedError", func() { Expect(makeError).To(MatchError(ccerror.ServiceInstanceParametersFetchNotSupportedError{ Message: "This service does not support fetching service instance parameters."})) }) diff --git a/command/v7/actor.go b/command/v7/actor.go index 2f29be698a1..57b6a1fff5c 100644 --- a/command/v7/actor.go +++ b/command/v7/actor.go @@ -155,7 +155,7 @@ type Actor interface { GetServiceKeyDetailsByServiceInstanceAndName(serviceInstanceName, serviceKeyName, spaceGUID string) (resources.ServiceCredentialBindingDetails, v7action.Warnings, error) GetServiceInstanceByNameAndSpace(serviceInstanceName, spaceGUID string) (resources.ServiceInstance, v7action.Warnings, error) GetServiceInstanceDetails(serviceInstanceName, spaceGUID string, omitApps bool) (v7action.ServiceInstanceDetails, v7action.Warnings, error) - GetServiceInstanceParameters(serviceInstanceName, spaceGUID string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) + GetServiceInstanceParameters(serviceInstanceName, spaceGUID string) (v7action.ServiceInstanceParameters, v7action.Warnings, error) GetServiceInstanceLabels(serviceInstanceName, spaceGUID string) (map[string]types.NullString, v7action.Warnings, error) GetServiceInstancesForSpace(spaceGUID string, omitApps bool) ([]v7action.ServiceInstance, v7action.Warnings, error) GetServiceKeysByServiceInstance(serviceInstanceName, spaceGUID string) ([]resources.ServiceCredentialBinding, v7action.Warnings, error) diff --git a/command/v7/service_command.go b/command/v7/service_command.go index e492a626d07..9d0fbd787c8 100644 --- a/command/v7/service_command.go +++ b/command/v7/service_command.go @@ -5,9 +5,10 @@ import ( "fmt" "strconv" + "code.cloudfoundry.org/cli/resources" + "code.cloudfoundry.org/cli/actor/v7action" "code.cloudfoundry.org/cli/command/flag" - "code.cloudfoundry.org/cli/resources" "code.cloudfoundry.org/cli/util/ui" ) @@ -20,35 +21,53 @@ type ServiceCommand struct { usage interface{} `usage:"CF_NAME service SERVICE_INSTANCE"` relatedCommands interface{} `related_commands:"bind-service, rename-service, update-service"` - serviceInstance v7action.ServiceInstanceDetails + serviceInstance v7action.ServiceInstanceDetails + serviceInstanceParams v7action.ServiceInstanceParameters } func (cmd ServiceCommand) Execute(args []string) error { if err := cmd.SharedActor.CheckTarget(true, true); err != nil { return err } - - var ( - warnings v7action.Warnings - err error - ) - switch { + case cmd.ShowGUID: + return cmd.fetchAndDisplayGUID() case cmd.Params: - cmd.serviceInstance, warnings, err = cmd.Actor.GetServiceInstanceParameters(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID) + return cmd.fetchAndDisplayParams() default: - cmd.serviceInstance, warnings, err = cmd.Actor.GetServiceInstanceDetails(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID, false) + return cmd.fetchAndDisplayDetails() } - cmd.UI.DisplayWarnings(warnings) +} + +func (cmd ServiceCommand) fetchAndDisplayGUID() error { + var err error + cmd.serviceInstance, err = cmd.fetchServiceInstanceDetails() + if err != nil { + return err + } + + cmd.UI.DisplayText(cmd.serviceInstance.GUID) + return nil +} + +func (cmd ServiceCommand) fetchAndDisplayParams() error { + var err error + cmd.serviceInstanceParams, err = cmd.fetchServiceInstanceParameters() + if err != nil { + return err + } + + return cmd.displayParameters() +} + +func (cmd ServiceCommand) fetchAndDisplayDetails() error { + var err error + cmd.serviceInstance, err = cmd.fetchServiceInstanceDetails() if err != nil { return err } switch { - case cmd.ShowGUID: - return cmd.displayGUID() - case cmd.Params: - return cmd.displayParameters() case cmd.serviceInstance.Type == resources.UserProvidedServiceInstance: return cmd.chain( cmd.displayIntro, @@ -67,9 +86,18 @@ func (cmd ServiceCommand) Execute(args []string) error { } } -func (cmd ServiceCommand) displayGUID() error { - cmd.UI.DisplayText(cmd.serviceInstance.GUID) - return nil +func (cmd ServiceCommand) fetchServiceInstanceDetails() (v7action.ServiceInstanceDetails, error) { + serviceInstance, warnings, err := cmd.Actor.GetServiceInstanceDetails(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID, false) + cmd.UI.DisplayWarnings(warnings) + + return serviceInstance, err +} + +func (cmd ServiceCommand) fetchServiceInstanceParameters() (v7action.ServiceInstanceParameters, error) { + serviceInstanceParameters, warnings, err := cmd.Actor.GetServiceInstanceParameters(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID) + cmd.UI.DisplayWarnings(warnings) + + return serviceInstanceParameters, err } func (cmd ServiceCommand) displayPropertiesUserProvided() error { @@ -163,7 +191,7 @@ func (cmd ServiceCommand) displayLastOperation() error { func (cmd ServiceCommand) displayParameters() error { switch { - case len(cmd.serviceInstance.Parameters.Value) > 0: + case len(cmd.serviceInstanceParams) > 0: cmd.displayParametersData() default: cmd.displayParametersEmpty() @@ -173,19 +201,13 @@ func (cmd ServiceCommand) displayParameters() error { } func (cmd ServiceCommand) displayParametersEmpty() { - cmd.UI.DisplayTextWithFlavor( - "No parameters are set for service instance {{.ServiceInstanceName}}.", - map[string]interface{}{ - "ServiceInstanceName": cmd.serviceInstance.Name, - }, + cmd.UI.DisplayText( + "No parameters are set for this service instance.", ) } func (cmd ServiceCommand) displayParametersData() { - data, err := json.MarshalIndent(cmd.serviceInstance.Parameters.Value, "", " ") - if err != nil { - panic(err) - } + data, _ := json.MarshalIndent(cmd.serviceInstanceParams, "", " ") cmd.UI.DisplayText(string(data)) } diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go index 89ba7ef323c..19523e304ec 100644 --- a/command/v7/service_command_test.go +++ b/command/v7/service_command_test.go @@ -611,9 +611,6 @@ var _ = Describe("service command", func() { }, ServicePlan: resources.ServicePlan{Name: servicePlanName}, ServiceBrokerName: serviceBrokerName, - Parameters: v7action.ServiceInstanceParameters{ - Value: types.JSONObject{"foo": "bar"}, - }, SharedStatus: v7action.SharedStatus{ IsSharedToOtherSpaces: true, }, @@ -654,10 +651,6 @@ var _ = Describe("service command", func() { }) When("the --params flag is specified", func() { - const ( - parameters = `{"foo":"bar"}` - ) - BeforeEach(func() { setFlag(&cmd, "--params") }) @@ -665,15 +658,7 @@ var _ = Describe("service command", func() { When("parameters are set", func() { BeforeEach(func() { fakeActor.GetServiceInstanceParametersReturns( - v7action.ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - GUID: serviceInstanceGUID, - Name: serviceInstanceName, - }, - Parameters: v7action.ServiceInstanceParameters{ - Value: types.JSONObject{"foo": "bar"}, - }, - }, + v7action.ServiceInstanceParameters(types.JSONObject{"foo": "bar"}), v7action.Warnings{"warning one", "warning two"}, nil, ) @@ -702,14 +687,7 @@ var _ = Describe("service command", func() { When("there was a problem retrieving the parameters", func() { BeforeEach(func() { fakeActor.GetServiceInstanceParametersReturns( - v7action.ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - GUID: serviceInstanceGUID, - Name: serviceInstanceName, - Type: resources.ManagedServiceInstance, - }, - Parameters: v7action.ServiceInstanceParameters{}, - }, + v7action.ServiceInstanceParameters{}, v7action.Warnings{"warning one", "warning two"}, errors.New("something happened"), ) @@ -727,16 +705,7 @@ var _ = Describe("service command", func() { When("the parameters are empty", func() { BeforeEach(func() { fakeActor.GetServiceInstanceParametersReturns( - v7action.ServiceInstanceDetails{ - ServiceInstance: resources.ServiceInstance{ - GUID: serviceInstanceGUID, - Name: serviceInstanceName, - Type: resources.ManagedServiceInstance, - }, - Parameters: v7action.ServiceInstanceParameters{ - Value: types.JSONObject{}, - }, - }, + v7action.ServiceInstanceParameters{}, v7action.Warnings{"warning one", "warning two"}, nil, ) @@ -744,7 +713,7 @@ var _ = Describe("service command", func() { It("says there were no parameters", func() { Expect(executeErr).NotTo(HaveOccurred()) - Expect(testUI.Out).To(Say(`No parameters are set for service instance %s.\n`, serviceInstanceName)) + Expect(testUI.Out).To(Say(`No parameters are set for this service instance.\n`)) }) }) }) diff --git a/command/v7/v7fakes/fake_actor.go b/command/v7/v7fakes/fake_actor.go index 7a5067cc813..dbe355f05db 100644 --- a/command/v7/v7fakes/fake_actor.go +++ b/command/v7/v7fakes/fake_actor.go @@ -2046,19 +2046,19 @@ type FakeActor struct { result2 v7action.Warnings result3 error } - GetServiceInstanceParametersStub func(string, string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) + GetServiceInstanceParametersStub func(string, string) (v7action.ServiceInstanceParameters, v7action.Warnings, error) getServiceInstanceParametersMutex sync.RWMutex getServiceInstanceParametersArgsForCall []struct { arg1 string arg2 string } getServiceInstanceParametersReturns struct { - result1 v7action.ServiceInstanceDetails + result1 v7action.ServiceInstanceParameters result2 v7action.Warnings result3 error } getServiceInstanceParametersReturnsOnCall map[int]struct { - result1 v7action.ServiceInstanceDetails + result1 v7action.ServiceInstanceParameters result2 v7action.Warnings result3 error } @@ -12263,7 +12263,7 @@ func (fake *FakeActor) GetServiceInstanceLabelsReturnsOnCall(i int, result1 map[ }{result1, result2, result3} } -func (fake *FakeActor) GetServiceInstanceParameters(arg1 string, arg2 string) (v7action.ServiceInstanceDetails, v7action.Warnings, error) { +func (fake *FakeActor) GetServiceInstanceParameters(arg1 string, arg2 string) (v7action.ServiceInstanceParameters, v7action.Warnings, error) { fake.getServiceInstanceParametersMutex.Lock() ret, specificReturn := fake.getServiceInstanceParametersReturnsOnCall[len(fake.getServiceInstanceParametersArgsForCall)] fake.getServiceInstanceParametersArgsForCall = append(fake.getServiceInstanceParametersArgsForCall, struct { @@ -12288,7 +12288,7 @@ func (fake *FakeActor) GetServiceInstanceParametersCallCount() int { return len(fake.getServiceInstanceParametersArgsForCall) } -func (fake *FakeActor) GetServiceInstanceParametersCalls(stub func(string, string) (v7action.ServiceInstanceDetails, v7action.Warnings, error)) { +func (fake *FakeActor) GetServiceInstanceParametersCalls(stub func(string, string) (v7action.ServiceInstanceParameters, v7action.Warnings, error)) { fake.getServiceInstanceParametersMutex.Lock() defer fake.getServiceInstanceParametersMutex.Unlock() fake.GetServiceInstanceParametersStub = stub @@ -12301,30 +12301,30 @@ func (fake *FakeActor) GetServiceInstanceParametersArgsForCall(i int) (string, s return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeActor) GetServiceInstanceParametersReturns(result1 v7action.ServiceInstanceDetails, result2 v7action.Warnings, result3 error) { +func (fake *FakeActor) GetServiceInstanceParametersReturns(result1 v7action.ServiceInstanceParameters, result2 v7action.Warnings, result3 error) { fake.getServiceInstanceParametersMutex.Lock() defer fake.getServiceInstanceParametersMutex.Unlock() fake.GetServiceInstanceParametersStub = nil fake.getServiceInstanceParametersReturns = struct { - result1 v7action.ServiceInstanceDetails + result1 v7action.ServiceInstanceParameters result2 v7action.Warnings result3 error }{result1, result2, result3} } -func (fake *FakeActor) GetServiceInstanceParametersReturnsOnCall(i int, result1 v7action.ServiceInstanceDetails, result2 v7action.Warnings, result3 error) { +func (fake *FakeActor) GetServiceInstanceParametersReturnsOnCall(i int, result1 v7action.ServiceInstanceParameters, result2 v7action.Warnings, result3 error) { fake.getServiceInstanceParametersMutex.Lock() defer fake.getServiceInstanceParametersMutex.Unlock() fake.GetServiceInstanceParametersStub = nil if fake.getServiceInstanceParametersReturnsOnCall == nil { fake.getServiceInstanceParametersReturnsOnCall = make(map[int]struct { - result1 v7action.ServiceInstanceDetails + result1 v7action.ServiceInstanceParameters result2 v7action.Warnings result3 error }) } fake.getServiceInstanceParametersReturnsOnCall[i] = struct { - result1 v7action.ServiceInstanceDetails + result1 v7action.ServiceInstanceParameters result2 v7action.Warnings result3 error }{result1, result2, result3} diff --git a/integration/v7/isolated/service_command_test.go b/integration/v7/isolated/service_command_test.go index 04e1e027fab..84ad96b4232 100644 --- a/integration/v7/isolated/service_command_test.go +++ b/integration/v7/isolated/service_command_test.go @@ -80,7 +80,7 @@ var _ = Describe("service command", func() { }) }) - When("user is logged in and targeting a space", func() { + FWhen("user is logged in and targeting a space", func() { var ( serviceInstanceName string orgName string From d954c1b02a5ad428dec455a6bdf2fcbd85257a74 Mon Sep 17 00:00:00 2001 From: George Blue <gblue@pivotal.io> Date: Thu, 18 Feb 2021 17:44:47 +0000 Subject: [PATCH 6/6] v8(services): simplify code to display service --- command/v7/service_command.go | 250 +++++++----------- command/v7/service_command_test.go | 43 ++- .../v7/isolated/service_command_test.go | 2 +- 3 files changed, 134 insertions(+), 161 deletions(-) diff --git a/command/v7/service_command.go b/command/v7/service_command.go index 9d0fbd787c8..9861aaa0f0e 100644 --- a/command/v7/service_command.go +++ b/command/v7/service_command.go @@ -5,10 +5,9 @@ import ( "fmt" "strconv" - "code.cloudfoundry.org/cli/resources" - "code.cloudfoundry.org/cli/actor/v7action" "code.cloudfoundry.org/cli/command/flag" + "code.cloudfoundry.org/cli/resources" "code.cloudfoundry.org/cli/util/ui" ) @@ -20,9 +19,6 @@ type ServiceCommand struct { Params bool `long:"params" description:"Retrieve and display the given service instances's parameters. All other output is suppressed."` usage interface{} `usage:"CF_NAME service SERVICE_INSTANCE"` relatedCommands interface{} `related_commands:"bind-service, rename-service, update-service"` - - serviceInstance v7action.ServiceInstanceDetails - serviceInstanceParams v7action.ServiceInstanceParameters } func (cmd ServiceCommand) Execute(args []string) error { @@ -40,210 +36,180 @@ func (cmd ServiceCommand) Execute(args []string) error { } func (cmd ServiceCommand) fetchAndDisplayGUID() error { - var err error - cmd.serviceInstance, err = cmd.fetchServiceInstanceDetails() + serviceInstance, _, err := cmd.Actor.GetServiceInstanceByNameAndSpace( + string(cmd.RequiredArgs.ServiceInstance), + cmd.Config.TargetedSpace().GUID, + ) if err != nil { return err } - cmd.UI.DisplayText(cmd.serviceInstance.GUID) + cmd.UI.DisplayText(serviceInstance.GUID) return nil } func (cmd ServiceCommand) fetchAndDisplayParams() error { - var err error - cmd.serviceInstanceParams, err = cmd.fetchServiceInstanceParameters() + params, warnings, err := cmd.Actor.GetServiceInstanceParameters( + string(cmd.RequiredArgs.ServiceInstance), + cmd.Config.TargetedSpace().GUID, + ) + cmd.UI.DisplayWarnings(warnings) if err != nil { return err } - return cmd.displayParameters() + data, _ := json.MarshalIndent(params, "", " ") + cmd.UI.DisplayText(string(data)) + return nil } func (cmd ServiceCommand) fetchAndDisplayDetails() error { - var err error - cmd.serviceInstance, err = cmd.fetchServiceInstanceDetails() + if err := cmd.displayIntro(); err != nil { + return err + } + + serviceInstanceWithDetails, warnings, err := cmd.Actor.GetServiceInstanceDetails( + string(cmd.RequiredArgs.ServiceInstance), + cmd.Config.TargetedSpace().GUID, + false, + ) + cmd.UI.DisplayWarnings(warnings) if err != nil { return err } switch { - case cmd.serviceInstance.Type == resources.UserProvidedServiceInstance: - return cmd.chain( - cmd.displayIntro, - cmd.displayPropertiesUserProvided, - cmd.displayBoundApps, - ) + case serviceInstanceWithDetails.Type == resources.UserProvidedServiceInstance: + cmd.displayPropertiesUserProvided(serviceInstanceWithDetails) + cmd.displayBoundApps(serviceInstanceWithDetails) default: - return cmd.chain( - cmd.displayIntro, - cmd.displayPropertiesManaged, - cmd.displayLastOperation, - cmd.displayBoundApps, - cmd.displaySharingInfo, - cmd.displayUpgrades, - ) + cmd.displayPropertiesManaged(serviceInstanceWithDetails) + cmd.displayLastOperation(serviceInstanceWithDetails) + cmd.displayBoundApps(serviceInstanceWithDetails) + cmd.displaySharingInfo(serviceInstanceWithDetails) + cmd.displayUpgrades(serviceInstanceWithDetails) } -} -func (cmd ServiceCommand) fetchServiceInstanceDetails() (v7action.ServiceInstanceDetails, error) { - serviceInstance, warnings, err := cmd.Actor.GetServiceInstanceDetails(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID, false) - cmd.UI.DisplayWarnings(warnings) - - return serviceInstance, err + return nil } -func (cmd ServiceCommand) fetchServiceInstanceParameters() (v7action.ServiceInstanceParameters, error) { - serviceInstanceParameters, warnings, err := cmd.Actor.GetServiceInstanceParameters(string(cmd.RequiredArgs.ServiceInstance), cmd.Config.TargetedSpace().GUID) - cmd.UI.DisplayWarnings(warnings) +func (cmd ServiceCommand) displayIntro() error { + user, err := cmd.Config.CurrentUser() + if err != nil { + return err + } + + cmd.UI.DisplayTextWithFlavor( + "Showing info of service {{.ServiceInstanceName}} in org {{.OrgName}} / space {{.SpaceName}} as {{.Username}}...", + map[string]interface{}{ + "ServiceInstanceName": cmd.RequiredArgs.ServiceInstance, + "OrgName": cmd.Config.TargetedOrganization().Name, + "SpaceName": cmd.Config.TargetedSpace().Name, + "Username": user.Name, + }, + ) + cmd.UI.DisplayNewline() - return serviceInstanceParameters, err + return nil } -func (cmd ServiceCommand) displayPropertiesUserProvided() error { +func (cmd ServiceCommand) displayPropertiesUserProvided(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { table := [][]string{ - {cmd.UI.TranslateText("name:"), cmd.serviceInstance.Name}, - {cmd.UI.TranslateText("guid:"), cmd.serviceInstance.GUID}, - {cmd.UI.TranslateText("type:"), string(cmd.serviceInstance.Type)}, - {cmd.UI.TranslateText("tags:"), cmd.serviceInstance.Tags.String()}, - {cmd.UI.TranslateText("route service url:"), cmd.serviceInstance.RouteServiceURL.String()}, - {cmd.UI.TranslateText("syslog drain url:"), cmd.serviceInstance.SyslogDrainURL.String()}, + {cmd.UI.TranslateText("name:"), serviceInstanceWithDetails.Name}, + {cmd.UI.TranslateText("guid:"), serviceInstanceWithDetails.GUID}, + {cmd.UI.TranslateText("type:"), string(serviceInstanceWithDetails.Type)}, + {cmd.UI.TranslateText("tags:"), serviceInstanceWithDetails.Tags.String()}, + {cmd.UI.TranslateText("route service url:"), serviceInstanceWithDetails.RouteServiceURL.String()}, + {cmd.UI.TranslateText("syslog drain url:"), serviceInstanceWithDetails.SyslogDrainURL.String()}, } cmd.UI.DisplayKeyValueTable("", table, 3) - return nil + cmd.UI.DisplayNewline() } -func (cmd ServiceCommand) displayPropertiesManaged() error { +func (cmd ServiceCommand) displayPropertiesManaged(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { table := [][]string{ - {cmd.UI.TranslateText("name:"), cmd.serviceInstance.Name}, - {cmd.UI.TranslateText("guid:"), cmd.serviceInstance.GUID}, - {cmd.UI.TranslateText("type:"), string(cmd.serviceInstance.Type)}, - {cmd.UI.TranslateText("broker:"), cmd.serviceInstance.ServiceBrokerName}, - {cmd.UI.TranslateText("offering:"), cmd.serviceInstance.ServiceOffering.Name}, - {cmd.UI.TranslateText("plan:"), cmd.serviceInstance.ServicePlan.Name}, - {cmd.UI.TranslateText("tags:"), cmd.serviceInstance.Tags.String()}, - {cmd.UI.TranslateText("offering tags:"), cmd.serviceInstance.ServiceOffering.Tags.String()}, - {cmd.UI.TranslateText("description:"), cmd.serviceInstance.ServiceOffering.Description}, - {cmd.UI.TranslateText("documentation:"), cmd.serviceInstance.ServiceOffering.DocumentationURL}, - {cmd.UI.TranslateText("dashboard url:"), cmd.serviceInstance.DashboardURL.String()}, + {cmd.UI.TranslateText("name:"), serviceInstanceWithDetails.Name}, + {cmd.UI.TranslateText("guid:"), serviceInstanceWithDetails.GUID}, + {cmd.UI.TranslateText("type:"), string(serviceInstanceWithDetails.Type)}, + {cmd.UI.TranslateText("broker:"), serviceInstanceWithDetails.ServiceBrokerName}, + {cmd.UI.TranslateText("offering:"), serviceInstanceWithDetails.ServiceOffering.Name}, + {cmd.UI.TranslateText("plan:"), serviceInstanceWithDetails.ServicePlan.Name}, + {cmd.UI.TranslateText("tags:"), serviceInstanceWithDetails.Tags.String()}, + {cmd.UI.TranslateText("offering tags:"), serviceInstanceWithDetails.ServiceOffering.Tags.String()}, + {cmd.UI.TranslateText("description:"), serviceInstanceWithDetails.ServiceOffering.Description}, + {cmd.UI.TranslateText("documentation:"), serviceInstanceWithDetails.ServiceOffering.DocumentationURL}, + {cmd.UI.TranslateText("dashboard url:"), serviceInstanceWithDetails.DashboardURL.String()}, } cmd.UI.DisplayKeyValueTable("", table, 3) - - return nil + cmd.UI.DisplayNewline() } -func (cmd ServiceCommand) displaySharingInfo() error { +func (cmd ServiceCommand) displaySharingInfo(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { cmd.UI.DisplayText("Sharing:") cmd.UI.DisplayNewline() - sharedStatus := cmd.serviceInstance.SharedStatus - - if sharedStatus.IsSharedFromOriginalSpace { + if serviceInstanceWithDetails.SharedStatus.IsSharedFromOriginalSpace { cmd.UI.DisplayText("This service instance is shared from space {{.Space}} of org {{.Org}}.", map[string]interface{}{ - "Space": cmd.serviceInstance.SpaceName, - "Org": cmd.serviceInstance.OrganizationName, + "Space": serviceInstanceWithDetails.SpaceName, + "Org": serviceInstanceWithDetails.OrganizationName, }) cmd.UI.DisplayNewline() - return nil + return } - if sharedStatus.IsSharedToOtherSpaces { + if serviceInstanceWithDetails.SharedStatus.IsSharedToOtherSpaces { cmd.UI.DisplayText("Shared with spaces:") - cmd.displaySharedTo() + cmd.displaySharedTo(serviceInstanceWithDetails) } else { cmd.UI.DisplayText("This service instance is not currently being shared.") + cmd.UI.DisplayNewline() } - if sharedStatus.FeatureFlagIsDisabled { + if serviceInstanceWithDetails.SharedStatus.FeatureFlagIsDisabled { cmd.UI.DisplayText(`The "service_instance_sharing" feature flag is disabled for this Cloud Foundry platform.`) cmd.UI.DisplayNewline() } - if sharedStatus.OfferingDisablesSharing { + if serviceInstanceWithDetails.SharedStatus.OfferingDisablesSharing { cmd.UI.DisplayText("Service instance sharing is disabled for this service offering.") cmd.UI.DisplayNewline() } - - return nil } -func (cmd ServiceCommand) displayLastOperation() error { +func (cmd ServiceCommand) displayLastOperation(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { cmd.UI.DisplayTextWithFlavor( "Showing status of last operation from service instance {{.ServiceInstanceName}}...", map[string]interface{}{ - "ServiceInstanceName": cmd.serviceInstance.Name, + "ServiceInstanceName": serviceInstanceWithDetails.Name, }, ) cmd.UI.DisplayNewline() - status := fmt.Sprintf("%s %s", cmd.serviceInstance.LastOperation.Type, cmd.serviceInstance.LastOperation.State) + status := fmt.Sprintf("%s %s", serviceInstanceWithDetails.LastOperation.Type, serviceInstanceWithDetails.LastOperation.State) table := [][]string{ {cmd.UI.TranslateText("status:"), status}, - {cmd.UI.TranslateText("message:"), cmd.serviceInstance.LastOperation.Description}, - {cmd.UI.TranslateText("started:"), cmd.serviceInstance.LastOperation.CreatedAt}, - {cmd.UI.TranslateText("updated:"), cmd.serviceInstance.LastOperation.UpdatedAt}, + {cmd.UI.TranslateText("message:"), serviceInstanceWithDetails.LastOperation.Description}, + {cmd.UI.TranslateText("started:"), serviceInstanceWithDetails.LastOperation.CreatedAt}, + {cmd.UI.TranslateText("updated:"), serviceInstanceWithDetails.LastOperation.UpdatedAt}, } cmd.UI.DisplayKeyValueTable("", table, 3) - - return nil -} - -func (cmd ServiceCommand) displayParameters() error { - switch { - case len(cmd.serviceInstanceParams) > 0: - cmd.displayParametersData() - default: - cmd.displayParametersEmpty() - } - - return nil -} - -func (cmd ServiceCommand) displayParametersEmpty() { - cmd.UI.DisplayText( - "No parameters are set for this service instance.", - ) -} - -func (cmd ServiceCommand) displayParametersData() { - data, _ := json.MarshalIndent(cmd.serviceInstanceParams, "", " ") - - cmd.UI.DisplayText(string(data)) -} - -func (cmd ServiceCommand) displayIntro() error { - user, err := cmd.Config.CurrentUser() - if err != nil { - return err - } - - cmd.UI.DisplayTextWithFlavor( - "Showing info of service {{.ServiceInstanceName}} in org {{.OrgName}} / space {{.SpaceName}} as {{.Username}}...", - map[string]interface{}{ - "ServiceInstanceName": cmd.serviceInstance.Name, - "OrgName": cmd.Config.TargetedOrganization().Name, - "SpaceName": cmd.Config.TargetedSpace().Name, - "Username": user.Name, - }, - ) - - return nil + cmd.UI.DisplayNewline() } -func (cmd ServiceCommand) displayUpgrades() error { +func (cmd ServiceCommand) displayUpgrades(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { cmd.UI.DisplayText("Upgrading:") - switch cmd.serviceInstance.UpgradeStatus.State { + switch serviceInstanceWithDetails.UpgradeStatus.State { case v7action.ServiceInstanceUpgradeAvailable: cmd.UI.DisplayText("Showing available upgrade details for this service...") cmd.UI.DisplayNewline() cmd.UI.DisplayText("Upgrade description: {{.Description}}", map[string]interface{}{ - "Description": cmd.serviceInstance.UpgradeStatus.Description, + "Description": serviceInstanceWithDetails.UpgradeStatus.Description, }) cmd.UI.DisplayNewline() cmd.UI.DisplayText("TIP: You can upgrade using 'cf upgrade-service {{.InstanceName}}'", map[string]interface{}{ - "InstanceName": cmd.serviceInstance.Name, + "InstanceName": serviceInstanceWithDetails.Name, }) case v7action.ServiceInstanceUpgradeNotAvailable: cmd.UI.DisplayText("There is no upgrade available for this service.") @@ -252,28 +218,28 @@ func (cmd ServiceCommand) displayUpgrades() error { } cmd.UI.DisplayNewline() - return nil } -func (cmd ServiceCommand) displaySharedTo() error { +func (cmd ServiceCommand) displaySharedTo(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { table := [][]string{{"org", "space", "bindings"}} - for _, usageSummaryLine := range cmd.serviceInstance.SharedStatus.UsageSummary { + for _, usageSummaryLine := range serviceInstanceWithDetails.SharedStatus.UsageSummary { table = append(table, []string{usageSummaryLine.OrganizationName, usageSummaryLine.SpaceName, strconv.Itoa(usageSummaryLine.BoundAppCount)}) } cmd.UI.DisplayTableWithHeader(" ", table, ui.DefaultTableSpacePadding) - return nil + cmd.UI.DisplayNewline() } -func (cmd ServiceCommand) displayBoundApps() error { +func (cmd ServiceCommand) displayBoundApps(serviceInstanceWithDetails v7action.ServiceInstanceDetails) { cmd.UI.DisplayText("Bound apps:") - if len(cmd.serviceInstance.BoundApps) == 0 { + if len(serviceInstanceWithDetails.BoundApps) == 0 { cmd.UI.DisplayText("There are no bound apps for this service instance.") - return nil + cmd.UI.DisplayNewline() + return } table := [][]string{{"name", "binding name", "status", "message"}} - for _, app := range cmd.serviceInstance.BoundApps { + for _, app := range serviceInstanceWithDetails.BoundApps { table = append(table, []string{ app.AppName, app.Name, @@ -283,19 +249,5 @@ func (cmd ServiceCommand) displayBoundApps() error { } cmd.UI.DisplayTableWithHeader(" ", table, ui.DefaultTableSpacePadding) - return nil -} - -func (cmd ServiceCommand) chain(steps ...func() error) error { - for i, step := range steps { - if err := step(); err != nil { - return err - } - - if i < len(steps) { - cmd.UI.DisplayNewline() - } - } - - return nil + cmd.UI.DisplayNewline() } diff --git a/command/v7/service_command_test.go b/command/v7/service_command_test.go index 19523e304ec..72ec2d67ee0 100644 --- a/command/v7/service_command_test.go +++ b/command/v7/service_command_test.go @@ -91,23 +91,45 @@ var _ = Describe("service command", func() { When("the --guid flag is specified", func() { BeforeEach(func() { + fakeActor.GetServiceInstanceByNameAndSpaceReturns( + resources.ServiceInstance{ + GUID: serviceInstanceGUID, + Name: serviceInstanceName, + }, + v7action.Warnings{"warning one", "warning two"}, + nil, + ) + setFlag(&cmd, "--guid") }) - It("looks up the service instance and prints the GUID and warnings", func() { + It("looks up the service instance and prints the GUID and no warnings", func() { Expect(executeErr).NotTo(HaveOccurred()) - Expect(fakeActor.GetServiceInstanceDetailsCallCount()).To(Equal(1)) - actualName, actualSpaceGUID, actualOmitApps := fakeActor.GetServiceInstanceDetailsArgsForCall(0) + Expect(fakeActor.GetServiceInstanceByNameAndSpaceCallCount()).To(Equal(1)) + actualName, actualSpaceGUID := fakeActor.GetServiceInstanceByNameAndSpaceArgsForCall(0) Expect(actualName).To(Equal(serviceInstanceName)) Expect(actualSpaceGUID).To(Equal(spaceGUID)) - Expect(actualOmitApps).To(BeFalse()) Expect(testUI.Out).To(Say(`^%s\n$`, serviceInstanceGUID)) - Expect(testUI.Err).To(SatisfyAll( - Say("warning one"), - Say("warning two"), - )) + Expect(testUI.Err).NotTo(Say("warning")) + }) + + When("getting the service instance fails", func() { + BeforeEach(func() { + fakeActor.GetServiceInstanceByNameAndSpaceReturns( + resources.ServiceInstance{ + GUID: serviceInstanceGUID, + Name: serviceInstanceName, + }, + v7action.Warnings{"warning one", "warning two"}, + errors.New("yuck"), + ) + }) + + It("returns the error", func() { + Expect(executeErr).To(MatchError("yuck")) + }) }) }) @@ -711,9 +733,9 @@ var _ = Describe("service command", func() { ) }) - It("says there were no parameters", func() { + It("displays empty parameters", func() { Expect(executeErr).NotTo(HaveOccurred()) - Expect(testUI.Out).To(Say(`No parameters are set for this service instance.\n`)) + Expect(testUI.Out).To(Say(`{}\n`)) }) }) }) @@ -730,7 +752,6 @@ var _ = Describe("service command", func() { It("prints warnings and returns an error", func() { Expect(executeErr).To(MatchError("boom")) - Expect(testUI.Out).NotTo(Say(`.`), "output not empty!") Expect(testUI.Err).To(SatisfyAll( Say("warning one"), Say("warning two"), diff --git a/integration/v7/isolated/service_command_test.go b/integration/v7/isolated/service_command_test.go index 84ad96b4232..04e1e027fab 100644 --- a/integration/v7/isolated/service_command_test.go +++ b/integration/v7/isolated/service_command_test.go @@ -80,7 +80,7 @@ var _ = Describe("service command", func() { }) }) - FWhen("user is logged in and targeting a space", func() { + When("user is logged in and targeting a space", func() { var ( serviceInstanceName string orgName string