Remove the state from service brokers API

[#172646313](https://www.pivotaltracker.com/story/show/172646313)

Author: pivotal-marcela-campo

app/presenters/v3/service_broker_presenter.rb

@@ -10,20 +10,11 @@ module V3
       class ServiceBrokerPresenter < BasePresenter
         include VCAP::CloudController::Presenters::Mixins::MetadataPresentationHelpers
 
-        STATES = {
-          VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING => 'synchronization in progress',
-          VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED => 'synchronization failed',
-          VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE => 'available',
-          VCAP::CloudController::ServiceBrokerStateEnum::DELETE_IN_PROGRESS => 'delete in progress',
-          VCAP::CloudController::ServiceBrokerStateEnum::DELETE_FAILED => 'delete failed'
-        }.tap { |s| s.default = 'available' }.freeze
-
         def to_hash
           {
             guid: broker.guid,
             name: broker.name,
             url: broker.broker_url,
-            status: status,
             created_at: broker.created_at,
             updated_at: broker.updated_at,
             relationships: build_relationships,
@@ -41,10 +32,6 @@ def broker
           @resource
         end
 
-        def status
-          STATES[broker.state]
-        end
-
         def build_relationships
           if broker.space_guid.nil?
             {}

docs/v3/source/includes/experimental_resources/service_brokers/_create.md.erb

@@ -39,7 +39,9 @@ Content-Type: application/json
 Location: https://api.example.org/v3/jobs/af5c57f6-8769-41fa-a499-2c84ed896788
 ```
 
-This endpoint creates a new service broker and a job to synchronize the service offerings and service plans with those in the broker's catalog.  The `Location` header refers to the created job which syncs the broker with the catalog.
+This endpoint creates a new service broker and a job to synchronize the service offerings and service plans with those in the broker's catalog.
+The `Location` header refers to the created job which syncs the broker with the catalog. See [_Service broker jobs_](#service-broker-jobs) for more information and limitations.
+
 
 #### Definition
 `POST /v3/service_brokers`

docs/v3/source/includes/experimental_resources/service_brokers/_delete.md.erb

@@ -20,7 +20,7 @@ Content-Type: application/json
 Location: https://api.example.org/v3/jobs/af5c57f6-8769-41fa-a499-2c84ed896788
 ```
 
-This endpoint creates a job to delete an existing service broker. The `Location` header refers to the created job.
+This endpoint creates a job to delete an existing service broker. The `Location` header refers to the created job. See [_Service broker jobs_](#service-broker-jobs) for more information and limitations.
 
 #### Definition
 `DELETE /v3/service_brokers/:guid`

docs/v3/source/includes/experimental_resources/service_brokers/_jobs.md.erb

@@ -0,0 +1,11 @@
+### Service Broker Jobs
+
+CRUD operations for service brokers are asynchronous. `create`, `update` and `delete` endpoints return urls to `jobs` in the `Location` header of the HTTP response.
+
+These jobs are the only source of information about the progress and end state of the operation. Therefore, the job must be polled to find out when the process ends and if it was successful.
+It's also worth noting that there is no mechanism to link a service broker with its jobs, apart from the `Location` header in the API response.
+
+When a failure occurs during a create job, then the service broker won't be usable. However it will still be included in the `GET` endpoints responses, hence the importance of querying the job to find out the end status.
+
+When updating a service broker, the changes will be rollbacked in the event the job fails. The service broker will remain unchanged with no indication that the update operation failed. Querying the job will provide such information.
+

docs/v3/source/includes/experimental_resources/service_brokers/_object.md.erb

@@ -12,7 +12,6 @@ Name | Type | Description
 **guid** | _uuid_ | Unique identifier for the service broker.
 **name** | _string_ | Name of the service broker.
 **url** | _string_ | URL of the service broker.
-**status** | _string_ | Availability status of the service broker.
 **created_at** | _datetime_ | The time with zone when the object was created.
 **updated_at** | _datetime_ | The time with zone when the object was last updated.
 **relationships.space** | [_to-one relationship_](#to-one-relationships) | The space the service broker is restricted to. Omitted for globally available service brokers.

docs/v3/source/includes/experimental_resources/service_brokers/_update.md.erb

@@ -36,7 +36,9 @@ Content-Type: application/json
 Location: https://api.example.org/v3/jobs/af5c57f6-8769-41fa-a499-2c84ed896788
 ```
 
-This endpoint updates a broker and creates a job to synchronize the service offerings and service plans with those in the broker's catalog.  The `Location` header refers to the created job which syncs the broker with the catalog.
+This endpoint updates a broker and creates a job to synchronize the service offerings and service plans with those in the broker's catalog.
+The `Location` header refers to the created job which syncs the broker with the catalog. See [_Service broker jobs_](#service-broker-jobs) for more information and limitations.
+
 Service Brokers that are in the process of being synchronized or deleted cannot be updated.
 
 #### Definition

docs/v3/source/index.md

@@ -312,6 +312,7 @@ includes:
   - experimental_resources/service_brokers/list
   - experimental_resources/service_brokers/update
   - experimental_resources/service_brokers/delete
+  - experimental_resources/service_brokers/jobs
   - experimental_resources/service_offerings/header
   - experimental_resources/service_offerings/visibility
   - experimental_resources/service_offerings/object

spec/request/lifecycle/service_brokers_spec.rb

@@ -46,29 +46,11 @@
         expect(parsed_response['state']).to eq('PROCESSING')
         broker = parsed_response.dig('links', 'service_brokers', 'href')
 
-        # Check it's in progress
-        get broker, {}, admin_headers
-        expect(last_response).to have_status_code(200)
-        expect(parsed_response).to include(
-          'name' => 'my-service-broker',
-          'url' => 'http://example.org/my-service-broker-url',
-          'status' => 'synchronization in progress',
-        )
-
         # Check it finishes
         execute_all_jobs(expected_successes: 1, expected_failures: 0)
         get job_url_for_create, {}, admin_headers
         expect(last_response).to have_status_code(200)
         expect(parsed_response['state']).to eq('COMPLETE')
-
-        # Check it's correct
-        get broker, {}, admin_headers
-        expect(last_response).to have_status_code(200)
-        expect(parsed_response).to include(
-          'name' => 'my-service-broker',
-          'url' => 'http://example.org/my-service-broker-url',
-          'status' => 'available',
-        )
       end
     end
 
@@ -90,15 +72,6 @@
         get job_url_for_create, {}, admin_headers
         expect(last_response).to have_status_code(200)
         expect(parsed_response['state']).to eq('FAILED')
-
-        # Check it's in failed state
-        get parsed_response.dig('links', 'service_brokers', 'href'), {}, admin_headers
-        expect(last_response).to have_status_code(200)
-        expect(parsed_response).to include(
-          'name' => 'my-service-broker',
-          'url' => 'http://example.org/my-service-broker-url',
-          'status' => 'synchronization failed',
-        )
       end
     end
 
@@ -189,7 +162,7 @@
               type: 'basic',
               credentials: {
                   username: 'old-admin',
-                  password: 'not-welcome',
+                  password: 'not-welcome'
               }
           },
           metadata: {
@@ -207,7 +180,7 @@
               type: 'basic',
               credentials: {
                   username: 'admin',
-                  password: 'welcome',
+                  password: 'welcome'
               }
           },
           metadata: {
@@ -235,7 +208,7 @@
             'state' => 'PROCESSING',
             'operation' => 'service_broker.update',
             'errors' => [],
-            'warnings' => [],
+            'warnings' => []
         })
 
         execute_all_jobs(expected_successes: 1, expected_failures: 0)
@@ -255,8 +228,7 @@
         expect(last_response).to have_status_code(200)
         expect(parsed_response).to include({
             'name' => 'new-name',
-            'url' => 'http://example.org/new-broker-url',
-            'status' => 'available',
+            'url' => 'http://example.org/new-broker-url'
         })
       end
 
@@ -297,7 +269,6 @@
       expect(parsed_response).to include({
           'name' => 'old-name',
           'url' => 'http://example.org/old-broker-url',
-          'status' => 'available',
           'metadata' => {
               'annotations' => {
                   'to_delete' => 'value',

spec/request/service_brokers_spec.rb

@@ -152,7 +152,6 @@
             url: global_service_broker_v3.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'available',
             relationships: {},
             links: {
               self: {
@@ -172,7 +171,6 @@
             url: global_service_broker_v2.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'available',
             relationships: {},
             links: {
               self: {
@@ -211,7 +209,6 @@
             url: space_scoped_service_broker.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'available',
             metadata: { labels: {}, annotations: {} },
             relationships: {
                 space: { data: { guid: space.guid } }
@@ -354,7 +351,6 @@ def expect_empty_list(user_headers)
             url: global_service_broker_v3.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'available',
             metadata: { labels: {}, annotations: {} },
             relationships: {},
             links: {
@@ -401,7 +397,6 @@ def expect_empty_list(user_headers)
             url: space_scoped_service_broker.broker_url,
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'available',
             metadata: { labels: {}, annotations: {} },
             relationships: {
                 space: { data: { guid: space.guid } }
@@ -710,7 +705,6 @@ def expect_empty_list(user_headers)
           url: 'http://example.org/broker-url',
           created_at: iso8601,
           updated_at: iso8601,
-          status: 'synchronization in progress',
           metadata: { labels: { potato: 'yam' }, annotations: { style: 'mashed' } },
           relationships: {},
           links: {
@@ -760,7 +754,7 @@ def expect_empty_list(user_headers)
       expect(last_response.headers['Location']).to end_with("/v3/jobs/#{job.guid}")
     end
 
-    it 'creates the services and the plans in the database' do
+    it 'creates the services and the plans in the database and updates broker state' do
       create_broker_successfully(global_broker_request_body, with: admin_headers)
       execute_all_jobs(expected_successes: 1, expected_failures: 0)
 
@@ -848,7 +842,6 @@ def expect_empty_list(user_headers)
             url: 'http://example.org/space-broker-url',
             created_at: iso8601,
             updated_at: iso8601,
-            status: 'synchronization in progress',
             metadata: { labels: {}, annotations: {} },
             relationships: {
                 space: { data: { guid: space.guid } }
@@ -1015,10 +1008,8 @@ def expect_empty_list(user_headers)
       let(:job) { VCAP::CloudController::PollableJobModel.last }
 
       it 'leaves broker in a non-available failed state' do
-        expect_broker_status(
-          status: 'synchronization failed',
-          with: admin_headers
-        )
+        broker = VCAP::CloudController::ServiceBroker.last
+        expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED)
       end
 
       it 'has failed the job with an appropriate error' do
@@ -1164,7 +1155,6 @@ def expect_created_broker(expected_broker)
       expect(VCAP::CloudController::ServiceBroker.count).to eq(@count_before_creation + 1)
 
       service_broker = VCAP::CloudController::ServiceBroker.last
-
       expect(service_broker).to include(
         'name' => expected_broker[:name],
         'broker_url' => expected_broker[:url],
@@ -1176,17 +1166,6 @@ def expect_created_broker(expected_broker)
       expect(service_broker.auth_password).to eq(expected_broker[:authentication][:credentials][:password])
     end
 
-    def expect_broker_status(status:, with:)
-      expect(VCAP::CloudController::ServiceBroker.count).to eq(@count_before_creation + 1)
-      service_broker = VCAP::CloudController::ServiceBroker.last
-
-      get("/v3/service_brokers/#{service_broker.guid}", {}, with)
-      expect(last_response.status).to eq(200)
-      expect(parsed_response).to include(
-        'status' => status
-      )
-    end
-
     def expect_no_broker_created
       expect(VCAP::CloudController::ServiceBroker.count).to eq(@count_before_creation)
     end
@@ -1206,11 +1185,10 @@ def assert_broker_state(broker_json)
       get broker_url, {}, admin_headers
       expect(last_response.status).to eq(200)
 
-      updated_service_broker_json = broker_json.tap do |broker|
-        broker[:status] = 'available'
-      end
+      expect(parsed_response).to match_json_response(broker_json)
 
-      expect(parsed_response).to match_json_response(updated_service_broker_json)
+      service_broker = VCAP::CloudController::ServiceBroker.last
+      expect(service_broker.state).to eq('AVAILABLE')
     end
   end
 
@@ -1299,10 +1277,8 @@ def assert_broker_state(broker_json)
           end
 
           it 'marks the broker as deleting' do
-            get "/v3/service_brokers/#{global_broker.guid}", {}, admin_headers
-            expect(parsed_response).to include({
-                'status' => 'delete in progress'
-            })
+            broker = VCAP::CloudController::ServiceBroker.last
+            expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::DELETE_IN_PROGRESS)
           end
         end
 
@@ -1390,10 +1366,8 @@ def assert_broker_state(broker_json)
         end
 
         it 'updates the broker state' do
-          get "/v3/service_brokers/#{global_broker.guid}", {}, admin_headers
-          expect(parsed_response).to include({
-              'status' => 'delete failed'
-          })
+          broker = VCAP::CloudController::ServiceBroker.last
+          expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::DELETE_FAILED)
         end
       end
     end

spec/unit/presenters/v3/service_broker_presenter_spec.rb

@@ -18,7 +18,6 @@ module Presenters::V3
       end
 
       let(:service_broker_guid) { 'some-broker-guid' }
-      let(:service_broker_state) { nil }
       let(:space_guid) { nil }
       let(:service_broker) do
         double(
@@ -28,7 +27,6 @@ module Presenters::V3
           auth_username: 'username',
           auth_password: 'welcome',
           space_guid: space_guid,
-          state: service_broker_state,
           created_at: Time.now,
           updated_at: Time.now,
           labels: [label],
@@ -97,54 +95,6 @@ module Presenters::V3
             expect(result[:links][:space][:href]).to eq("#{link_prefix}/v3/spaces/#{space_guid}")
           end
         end
-
-        describe 'broker status' do
-          context 'when there is no state (e.g. legacy V2 created broker)' do
-            it 'is available' do
-              expect(result[:status]).to eq('available')
-            end
-          end
-
-          context 'when state is available' do
-            let(:service_broker_state) { ServiceBrokerStateEnum::AVAILABLE }
-
-            it 'is available' do
-              expect(result[:status]).to eq('available')
-            end
-          end
-
-          context 'when state is synchronizing' do
-            let(:service_broker_state) { ServiceBrokerStateEnum::SYNCHRONIZING }
-
-            it 'has synchronization in progress' do
-              expect(result[:status]).to eq('synchronization in progress')
-            end
-          end
-
-          context 'when state is synchronization failed' do
-            let(:service_broker_state) { ServiceBrokerStateEnum::SYNCHRONIZATION_FAILED }
-
-            it 'has synchronization failed' do
-              expect(result[:status]).to eq('synchronization failed')
-            end
-          end
-
-          context 'when state is delete in progress' do
-            let(:service_broker_state) { ServiceBrokerStateEnum::DELETE_IN_PROGRESS }
-
-            it 'has delete in progress' do
-              expect(result[:status]).to eq('delete in progress')
-            end
-          end
-
-          context 'when state is delete failed' do
-            let(:service_broker_state) { ServiceBrokerStateEnum::DELETE_FAILED }
-
-            it 'has delete failed' do
-              expect(result[:status]).to eq('delete failed')
-            end
-          end
-        end
       end
     end
   end