From 544525a3f707999f86c3169cf51dba2377e14cd1 Mon Sep 17 00:00:00 2001 From: George Blue Date: Tue, 12 Jan 2021 13:58:04 +0000 Subject: [PATCH] v3(services): revert synchronous broker name updates - the CF CLI always expects a job from a service broker rename - we therefore should always return a job, rather than HTTP 200 - it's ok to return HTTP 200 without a job for metadata updates as the CF CLI (cf set-label) will handle this [#169090128](https://www.pivotaltracker.com/story/show/169090128) --- app/actions/v3/service_broker_update.rb | 24 ++++++++-------- spec/request/service_brokers_spec.rb | 28 ++++++------------- .../actions/v3/service_broker_update_spec.rb | 8 ++---- 3 files changed, 23 insertions(+), 37 deletions(-) diff --git a/app/actions/v3/service_broker_update.rb b/app/actions/v3/service_broker_update.rb index 9053f3fc3b0..bd72c84b364 100644 --- a/app/actions/v3/service_broker_update.rb +++ b/app/actions/v3/service_broker_update.rb @@ -15,23 +15,31 @@ def initialize(service_broker, message, service_event_repository) @service_event_repository = service_event_repository end + # Technically a name change can be done without contacting the broker. However the CF CLI v7.2 + # (which is the most recent version at the time of writing) expects a broker rename to return a job. + # Once CF CLI v7.2 is out of support, then it may make sense to allow name changes to happen + # synchronously. def update_broker_needed? - message.requested?(:url) || message.requested?(:authentication) + message.requested?(:url) || message.requested?(:authentication) || message.requested?(:name) end def update_sync ServiceBroker.db.transaction do - broker.update(process_name(message)) MetadataUpdate.update(broker, message) end end def enqueue_update - params = process_name(message) + params = {} params[:broker_url] = message.url if message.requested?(:url) params[:authentication] = message.authentication.to_json if message.requested?(:authentication) params[:service_broker_id] = broker.id + if message.requested?(:name) + unique_name! if ServiceBroker.where(name: message.name).exclude(guid: broker.guid).any? + params[:name] = message.name + end + if broker.in_transitional_state? raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress') end @@ -57,16 +65,6 @@ def enqueue_update attr_reader :broker, :service_event_repository, :message - def process_name(message) - params = {} - if message.requested?(:name) - unique_name! if ServiceBroker.where(name: message.name).exclude(guid: broker.guid).any? - params[:name] = message.name - end - - params - end - def unique_name! raise InvalidServiceBroker.new('Name must be unique') end diff --git a/spec/request/service_brokers_spec.rb b/spec/request/service_brokers_spec.rb index 612d01a4aae..51325bce221 100644 --- a/spec/request/service_brokers_spec.rb +++ b/spec/request/service_brokers_spec.rb @@ -530,7 +530,7 @@ def expect_empty_list(user_headers) end end - context 'when updating url or authentication' do + context 'when updating url, authentication, or name' do let(:update_request_body) { { name: 'new-name', @@ -695,11 +695,9 @@ def expect_empty_list(user_headers) end end - context 'when updating name or metadata only' do - let(:new_name) { Sham.name } + context 'when metadata only' do let(:update_request_body) { { - name: new_name, metadata: { labels: { potato: 'sweet' }, annotations: { style: 'mashed', amount: 'all' } @@ -712,7 +710,7 @@ def expect_empty_list(user_headers) expect(last_response).to have_status_code(200) broker.reload - expect(broker.name).to eq(new_name) + expect(broker.name).to eq('broker name') expect(broker.broker_url).to eq('http://example.org/broker-url') expect(broker.auth_username).to eq('admin') expect(broker.auth_password).to eq('welcome') @@ -732,7 +730,7 @@ def expect_empty_list(user_headers) expect(parsed_response).to match_json_response( { guid: broker.guid, - name: new_name, + name: broker.name, url: broker.broker_url, created_at: iso8601, updated_at: iso8601, @@ -750,18 +748,6 @@ def expect_empty_list(user_headers) ) end - context 'when a broker with the same name exists' do - before do - VCAP::CloudController::ServiceBroker.make(name: 'another broker') - end - - it 'should return 422 and meaningful error and does not create a broker' do - patch("/v3/service_brokers/#{broker.guid}", { name: 'another broker' }.to_json, admin_headers) - expect_error(status: 422, error: 'UnprocessableEntity', description: 'Name must be unique') - expect(broker.reload.name).to eq 'broker name' - end - end - context 'when an operation is already in progress' do it 'updates the broker anyway' do broker.update(state: VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING) @@ -770,7 +756,11 @@ def expect_empty_list(user_headers) expect(last_response).to have_status_code(200) broker.reload - expect(broker.name).to eq(new_name) + expect(broker).to have_labels({ prefix: nil, key: 'potato', value: 'sweet' }) + expect(broker).to have_annotations( + { prefix: nil, key: 'style', value: 'mashed' }, + { prefix: nil, key: 'amount', value: 'all' } + ) expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::SYNCHRONIZING) end end diff --git a/spec/unit/actions/v3/service_broker_update_spec.rb b/spec/unit/actions/v3/service_broker_update_spec.rb index 56299c4fe4f..1a231442db8 100644 --- a/spec/unit/actions/v3/service_broker_update_spec.rb +++ b/spec/unit/actions/v3/service_broker_update_spec.rb @@ -30,8 +30,8 @@ module CloudController context 'name' do let(:request) { { name: 'new-name' } } - it 'is false' do - expect(action.update_broker_needed?).to be_falsey + it 'is true' do + expect(action.update_broker_needed?).to be_truthy end end @@ -79,7 +79,6 @@ module CloudController describe '#update_sync' do let(:request) do { - name: 'new-name', metadata: { labels: { potato: 'yam' }, annotations: { style: 'mashed' } @@ -91,8 +90,7 @@ module CloudController action.update_sync end - it 'updates name and metadata' do - expect(existing_service_broker.name).to eq('new-name') + it 'updates metadata' do expect(existing_service_broker).to have_labels({ key: 'potato', value: 'yam' }) expect(existing_service_broker).to have_annotations({ key: 'style', value: 'mashed' }) end