From 5120c4de6a9c08cb8fcd88b3ab4192b5c869f1b1 Mon Sep 17 00:00:00 2001 From: Greg Cobb Date: Mon, 23 Jan 2023 07:52:15 -0800 Subject: [PATCH 1/3] Add "default" field to v3 stacks - The default stack is only configurable by operators. - New apps use the default stack, if no stack is specified in their lifecycle. - There was no way for API users to know what the default stack is on a given Cloud Foundry deployment. - Apps will not change their stack, unless the app lifecycle is updated to a different stack. The default stack does not affect existing applications. - This field may be useful for writing app deploy scripts that automatically update the app's stack to be the default stack. - This could also enable push workflows like `cf push my-app --default-stack` (or something like that). --- app/models/runtime/stack.rb | 6 ++ app/presenters/v3/stack_presenter.rb | 1 + .../source/includes/api_resources/_stacks.erb | 3 + .../includes/concepts/_lifecycles.md.erb | 4 + .../includes/resources/stacks/_object.md.erb | 1 + spec/request/stacks_spec.rb | 89 +++++++++---------- spec/unit/models/runtime/stack_spec.rb | 42 +++++++++ .../presenters/v3/stack_presenter_spec.rb | 2 + 8 files changed, 103 insertions(+), 45 deletions(-) diff --git a/app/models/runtime/stack.rb b/app/models/runtime/stack.rb index 0b349543fbb..03156706ca1 100644 --- a/app/models/runtime/stack.rb +++ b/app/models/runtime/stack.rb @@ -41,6 +41,12 @@ def before_destroy super end + def default? + self == Stack.default + rescue MissingDefaultStackError + false + end + def self.configure(file_path) @config_file = if file_path ConfigFile.new(file_path) diff --git a/app/presenters/v3/stack_presenter.rb b/app/presenters/v3/stack_presenter.rb index 0849d86353f..900b5c96df6 100644 --- a/app/presenters/v3/stack_presenter.rb +++ b/app/presenters/v3/stack_presenter.rb @@ -12,6 +12,7 @@ def to_hash updated_at: stack.updated_at, name: stack.name, description: stack.description, + default: stack.default?, metadata: { labels: hashified_labels(stack.labels), annotations: hashified_annotations(stack.annotations), diff --git a/docs/v3/source/includes/api_resources/_stacks.erb b/docs/v3/source/includes/api_resources/_stacks.erb index 7f8c97f61b3..62cd114a66a 100644 --- a/docs/v3/source/includes/api_resources/_stacks.erb +++ b/docs/v3/source/includes/api_resources/_stacks.erb @@ -5,6 +5,7 @@ "updated_at": "2018-11-09T22:43:28Z", "name": "my-stack", "description": "Here is my stack!", + "default": true, "metadata": { "labels": <%= metadata.fetch(:labels, {}).to_json(space: ' ', object_nl: ' ')%>, "annotations": <%= metadata.fetch(:annotations, {}).to_json(space: ' ', object_nl: ' ')%> @@ -40,6 +41,7 @@ "updated_at": "2018-11-09T22:43:28Z", "name": "my-stack-1", "description": "This is my first stack!", + "default": true, "metadata": { "labels": {}, "annotations": {} @@ -56,6 +58,7 @@ "updated_at": "2018-11-09T22:43:29Z", "name": "my-stack-2", "description": "This is my second stack!", + "default": false, "metadata": { "labels": {}, "annotations": {} diff --git a/docs/v3/source/includes/concepts/_lifecycles.md.erb b/docs/v3/source/includes/concepts/_lifecycles.md.erb index 45a91127d3c..6a04cf8feaa 100644 --- a/docs/v3/source/includes/concepts/_lifecycles.md.erb +++ b/docs/v3/source/includes/concepts/_lifecycles.md.erb @@ -30,6 +30,10 @@ This is the default lifecycle for Cloud Foundry for VMs. When staging an app wit compiled using a buildpack, resulting in a droplet. When running an app with this lifecycle, a container running a rootfs will be created and the droplet will be expanded inside that container to be executed. +If buildpacks are not specified, then Cloud Foundry will automatically detect a +compatible buildpack, based on the files in an app's package. If a stack is not +specified, then the app will default to the operator-configured default stack. + **Note**: This lifecycle is not supported on Cloud Foundry for Kubernetes. #### Buildpack lifecycle object diff --git a/docs/v3/source/includes/resources/stacks/_object.md.erb b/docs/v3/source/includes/resources/stacks/_object.md.erb index 4265b0e4d27..3e216b1c346 100644 --- a/docs/v3/source/includes/resources/stacks/_object.md.erb +++ b/docs/v3/source/includes/resources/stacks/_object.md.erb @@ -14,6 +14,7 @@ Name | Type | Description **updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated **name** | _string_ | The name of the stack **description** | _string_ | The description of the stack +**default** | _boolean_ | Whether the stack is configured to be the default stack for new applications. **metadata.labels** | [_label object_](#labels) | Labels applied to the stack **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the stack **links** | [_links object_](#links) | Links to related resources diff --git a/spec/request/stacks_spec.rb b/spec/request/stacks_spec.rb index 5f12567befa..07781443f9e 100644 --- a/spec/request/stacks_spec.rb +++ b/spec/request/stacks_spec.rb @@ -2,9 +2,13 @@ require 'request_spec_shared_examples' RSpec.describe 'Stacks Request' do + let(:stack_config_file) { File.join(Paths::FIXTURES, 'config/stacks.yml') } + let(:default_stack_name) { 'default-stack-name' } let(:org) { VCAP::CloudController::Organization.make(created_at: 3.days.ago) } let(:space) { VCAP::CloudController::Space.make(organization: org) } + before { VCAP::CloudController::Stack.configure(stack_config_file) } + describe 'GET /v3/stacks' do before { VCAP::CloudController::Stack.dataset.destroy } let(:user) { make_user } @@ -13,57 +17,44 @@ context 'lists all stacks' do it_behaves_like 'permissions for list endpoint', ALL_PERMISSIONS do - let(:stacks_response_object) do - { - 'pagination' => { - 'total_results' => 3, - 'total_pages' => 2, - 'first' => { - 'href' => "#{link_prefix}/v3/stacks?page=1&per_page=2" - }, - 'last' => { - 'href' => "#{link_prefix}/v3/stacks?page=2&per_page=2" - }, - 'next' => { - 'href' => "#{link_prefix}/v3/stacks?page=2&per_page=2" - }, - 'previous' => nil - }, - 'resources' => [ - { - 'name' => stack1.name, - 'description' => stack1.description, - 'guid' => stack1.guid, - 'metadata' => { 'labels' => {}, 'annotations' => {} }, - 'created_at' => iso8601, - 'updated_at' => iso8601, - 'links' => { - 'self' => { - 'href' => "#{link_prefix}/v3/stacks/#{stack1.guid}" - } + let(:stacks_response_objects) do + [ + { + 'name' => stack1.name, + 'description' => stack1.description, + 'guid' => stack1.guid, + 'default' => false, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/stacks/#{stack1.guid}" } - }, - { - 'name' => stack2.name, - 'description' => stack2.description, - 'guid' => stack2.guid, - 'metadata' => { 'labels' => {}, 'annotations' => {} }, - 'created_at' => iso8601, - 'updated_at' => iso8601, - 'links' => { - 'self' => { - 'href' => "#{link_prefix}/v3/stacks/#{stack2.guid}" - } + } + }, + { + 'name' => stack2.name, + 'description' => stack2.description, + 'guid' => stack2.guid, + 'default' => true, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/stacks/#{stack2.guid}" } } - ] - } + } + ] end + let(:expected_codes_and_responses) do - Hash.new(code: 200, response_object: stacks_response_object) + Hash.new(code: 200, response_objects: stacks_response_objects) end let!(:stack1) { VCAP::CloudController::Stack.make } - let!(:stack2) { VCAP::CloudController::Stack.make } + let!(:stack2) { VCAP::CloudController::Stack.make(name: default_stack_name) } end end @@ -95,7 +86,7 @@ context 'When stacks exist' do let!(:stack1) { VCAP::CloudController::Stack.make } - let!(:stack2) { VCAP::CloudController::Stack.make } + let!(:stack2) { VCAP::CloudController::Stack.make(name: default_stack_name) } let!(:stack3) { VCAP::CloudController::Stack.make } it 'returns a paginated list of stacks' do @@ -122,6 +113,7 @@ 'name' => stack1.name, 'description' => stack1.description, 'guid' => stack1.guid, + 'default' => false, 'metadata' => { 'labels' => {}, 'annotations' => {} }, 'created_at' => iso8601, 'updated_at' => iso8601, @@ -135,6 +127,7 @@ 'name' => stack2.name, 'description' => stack2.description, 'guid' => stack2.guid, + 'default' => true, 'metadata' => { 'labels' => {}, 'annotations' => {} }, 'created_at' => iso8601, 'updated_at' => iso8601, @@ -171,6 +164,7 @@ 'name' => stack1.name, 'description' => stack1.description, 'guid' => stack1.guid, + 'default' => false, 'metadata' => { 'labels' => {}, 'annotations' => {} }, 'created_at' => iso8601, 'updated_at' => iso8601, @@ -184,6 +178,7 @@ 'name' => stack3.name, 'description' => stack3.description, 'guid' => stack3.guid, + 'default' => false, 'metadata' => { 'labels' => {}, 'annotations' => {} }, 'created_at' => iso8601, 'updated_at' => iso8601, @@ -234,6 +229,7 @@ 'name' => stack1.name, 'description' => stack1.description, 'guid' => stack1.guid, + 'default' => false, 'metadata' => { 'labels' => { 'release' => 'stable' @@ -267,6 +263,7 @@ 'name' => stack.name, 'description' => stack.description, 'guid' => stack.guid, + 'default' => false, 'metadata' => { 'labels' => {}, 'annotations' => {} }, 'created_at' => iso8601, 'updated_at' => iso8601, @@ -581,6 +578,7 @@ { 'name' => 'the-name', 'description' => 'the-description', + 'default' => false, 'metadata' => { 'labels' => { 'potato' => 'yam' @@ -642,6 +640,7 @@ { 'name' => stack.name, 'description' => stack.description, + 'default' => false, 'metadata' => { 'labels' => { 'potato' => 'yam' diff --git a/spec/unit/models/runtime/stack_spec.rb b/spec/unit/models/runtime/stack_spec.rb index d19050335e0..efdf57324d0 100644 --- a/spec/unit/models/runtime/stack_spec.rb +++ b/spec/unit/models/runtime/stack_spec.rb @@ -146,6 +146,48 @@ module VCAP::CloudController end end + describe '#default?' do + before { Stack.configure(file) } + let(:stack) { Stack.make(name: name) } + let(:name) { 'mimi' } + + context 'when config was not set' do + before { Stack.configure(nil) } + + it 'raises config not specified error' do + expect { + stack.default? + }.to raise_error(Stack::MissingConfigFileError) + end + end + + context 'when config was set' do + before { Stack.dataset.destroy } + + context 'when the stack has the default name' do + let(:name) { 'default-stack-name' } + + it 'returns true' do + expect(stack.default?).to be true + end + end + + context 'when there is NO default stack' do + it 'returns false' do + expect(stack.default?).to be false + end + end + + context 'when stack does NOT have the default name' do + before { Stack.make(name: 'default-stack-name') } + + it 'returns false' do + expect(stack.default?).to be false + end + end + end + end + describe '#destroy' do let(:stack) { Stack.make } diff --git a/spec/unit/presenters/v3/stack_presenter_spec.rb b/spec/unit/presenters/v3/stack_presenter_spec.rb index f2465823aaf..7ad67d32e52 100644 --- a/spec/unit/presenters/v3/stack_presenter_spec.rb +++ b/spec/unit/presenters/v3/stack_presenter_spec.rb @@ -47,6 +47,7 @@ expect(result[:updated_at]).to eq(stack.updated_at) expect(result[:name]).to eq(stack.name) expect(result[:description]).to eq(stack.description) + expect(result[:default]).to eq(false) expect(result[:metadata][:labels]).to eq('release' => 'stable', 'canberra.au/potato' => 'mashed') expect(result[:metadata][:annotations]).to eq('altitude' => '14,412', 'maize' => 'hfcs') expect(result[:links][:self][:href]).to eq("#{link_prefix}/v3/stacks/#{stack.guid}") @@ -67,6 +68,7 @@ expect(result[:created_at]).to eq(stack.created_at) expect(result[:updated_at]).to eq(stack.updated_at) expect(result[:name]).to eq(stack.name) + expect(result[:default]).to eq(false) expect(result[:links][:self][:href]).to eq("#{link_prefix}/v3/stacks/#{stack.guid}") end end From ad46c34fcd6eb1397d98909fa51e5e548f344966 Mon Sep 17 00:00:00 2001 From: Greg Cobb Date: Tue, 24 Jan 2023 11:48:40 -0800 Subject: [PATCH 2/3] Add "default" filter to v3 stacks - Boolean-ish filter adopted from v3 security groups - Enable clients to discover which stack is the operator-configured default stack --- app/fetchers/stack_list_fetcher.rb | 7 ++++ app/messages/stacks_list_message.rb | 5 +-- .../includes/resources/stacks/_list.md.erb | 1 + spec/request/stacks_spec.rb | 39 +++++++++++++++++++ spec/unit/fetchers/stack_list_fetcher_spec.rb | 28 ++++++++++++- .../unit/messages/stacks_list_message_spec.rb | 15 ++++++- 6 files changed, 88 insertions(+), 7 deletions(-) diff --git a/app/fetchers/stack_list_fetcher.rb b/app/fetchers/stack_list_fetcher.rb index 0a888aca940..2ff34eebe04 100644 --- a/app/fetchers/stack_list_fetcher.rb +++ b/app/fetchers/stack_list_fetcher.rb @@ -15,6 +15,13 @@ def filter(message, dataset) dataset = dataset.where(name: message.names) end + if message.requested?(:default) + condition = { name: Stack.default.name }.yield_self do |c| + ActiveModel::Type::Boolean.new.cast(message.default) ? c : Sequel.~(c) + end + dataset = dataset.where(condition) + end + if message.requested?(:label_selector) dataset = LabelSelectorQueryGenerator.add_selector_queries( label_klass: StackLabelModel, diff --git a/app/messages/stacks_list_message.rb b/app/messages/stacks_list_message.rb index 06efea8b4af..6f6515e74d7 100644 --- a/app/messages/stacks_list_message.rb +++ b/app/messages/stacks_list_message.rb @@ -2,13 +2,12 @@ module VCAP::CloudController class StacksListMessage < MetadataListMessage - register_allowed_keys [ - :names, - ] + register_allowed_keys [:names, :default,] validates_with NoAdditionalParamsValidator validates :names, array: true, allow_nil: true + validates :default, inclusion: { in: %w(true false) }, allow_nil: true def self.from_params(params) super(params, %w(names)) diff --git a/docs/v3/source/includes/resources/stacks/_list.md.erb b/docs/v3/source/includes/resources/stacks/_list.md.erb index 569bdb7abb2..c0fedb0a2c3 100644 --- a/docs/v3/source/includes/resources/stacks/_list.md.erb +++ b/docs/v3/source/includes/resources/stacks/_list.md.erb @@ -31,6 +31,7 @@ Retrieve all stacks. Name | Type | Description ---- | ---- | ------------ **names** | _list of strings_ | Comma-delimited list of app names to filter by +**default** | _boolean_ | If true, only return the default stack **page** | _integer_ | Page to display; valid values are integers >= 1 **per_page** | _integer_ | Number of results per page;
valid values are 1 through 5000 **order_by** | _string_ | Value to sort by. Defaults to ascending; prepend with `-` to sort descending
Valid values are `created_at`, `updated_at`, and `name` diff --git a/spec/request/stacks_spec.rb b/spec/request/stacks_spec.rb index 07781443f9e..2f7caa7edd5 100644 --- a/spec/request/stacks_spec.rb +++ b/spec/request/stacks_spec.rb @@ -73,6 +73,7 @@ let(:params) do { names: ['foo', 'bar'], + default: true, page: '2', per_page: '10', order_by: 'updated_at', @@ -82,6 +83,7 @@ updated_ats: { gt: Time.now.utc.iso8601 }, } end + let!(:stack) { VCAP::CloudController::Stack.make(name: default_stack_name) } end context 'When stacks exist' do @@ -193,6 +195,43 @@ ) end + it 'returns a list of stacks filtered by whether they are default' do + get '/v3/stacks?default=true', nil, user_header + + expect(parsed_response).to be_a_response_like( + { + 'pagination' => { + 'total_results' => 1, + 'total_pages' => 1, + 'first' => { + 'href' => "#{link_prefix}/v3/stacks?default=true&page=1&per_page=50" + }, + 'last' => { + 'href' => "#{link_prefix}/v3/stacks?default=true&page=1&per_page=50" + }, + 'next' => nil, + 'previous' => nil + }, + 'resources' => [ + { + 'name' => stack2.name, + 'description' => stack2.description, + 'guid' => stack2.guid, + 'default' => true, + 'metadata' => { 'labels' => {}, 'annotations' => {} }, + 'created_at' => iso8601, + 'updated_at' => iso8601, + 'links' => { + 'self' => { + 'href' => "#{link_prefix}/v3/stacks/#{stack2.guid}" + } + } + } + ] + } + ) + end + context 'when there are labels' do let!(:stack1_label) { VCAP::CloudController::StackLabelModel.make( key_name: 'release', diff --git a/spec/unit/fetchers/stack_list_fetcher_spec.rb b/spec/unit/fetchers/stack_list_fetcher_spec.rb index 371c1ccd8a3..24f51a32364 100644 --- a/spec/unit/fetchers/stack_list_fetcher_spec.rb +++ b/spec/unit/fetchers/stack_list_fetcher_spec.rb @@ -3,15 +3,19 @@ module VCAP::CloudController RSpec.describe StackListFetcher do + let(:stack_config_file) { File.join(Paths::FIXTURES, 'config/stacks.yml') } + let(:default_stack_name) { 'default-stack-name' } let(:fetcher) { StackListFetcher } + before { VCAP::CloudController::Stack.configure(stack_config_file) } + describe '#fetch_all' do before do Stack.dataset.destroy end let!(:stack1) { Stack.make } - let!(:stack2) { Stack.make } + let!(:stack2) { Stack.make(name: default_stack_name) } let(:message) { StacksListMessage.from_params(filters) } subject { fetcher.fetch_all(message) } @@ -24,7 +28,7 @@ module VCAP::CloudController end end - context 'when the stacks are filtered' do + context 'when the stacks are filtered by name' do let(:filters) { { names: [stack1.name] } } it 'returns all of the desired stacks' do @@ -33,6 +37,26 @@ module VCAP::CloudController end end + context 'when the stacks are filtered by default-ness' do + context 'when true' do + let(:filters) { { default: 'true' } } + + it 'returns all of the desired stacks' do + expect(subject).to_not include(stack1) + expect(subject).to include(stack2) + end + end + + context 'when false' do + let(:filters) { { default: 'false' } } + + it 'returns all of the desired stacks' do + expect(subject).to include(stack1) + expect(subject).to_not include(stack2) + end + end + end + context 'when a label_selector is provided' do let(:message) { StacksListMessage.from_params({ 'label_selector' => 'key=value' }) } let!(:stack1label) { StackLabelModel.make(key_name: 'key', value: 'value', stack: stack1) } diff --git a/spec/unit/messages/stacks_list_message_spec.rb b/spec/unit/messages/stacks_list_message_spec.rb index e2cb770adac..9603c7cdf68 100644 --- a/spec/unit/messages/stacks_list_message_spec.rb +++ b/spec/unit/messages/stacks_list_message_spec.rb @@ -7,6 +7,7 @@ module VCAP::CloudController let(:params) do { 'names' => 'name1,name2', + 'default' => 'true', 'page' => 1, 'per_page' => 5, } @@ -17,6 +18,7 @@ module VCAP::CloudController expect(message).to be_a(StacksListMessage) expect(message.names).to eq(%w(name1 name2)) + expect(message.default).to eq('true') expect(message.page).to eq(1) expect(message.per_page).to eq(5) end @@ -25,6 +27,7 @@ module VCAP::CloudController message = StacksListMessage.from_params(params) expect(message.requested?(:names)).to be_truthy + expect(message.requested?(:default)).to be_truthy expect(message.requested?(:page)).to be_truthy expect(message.requested?(:per_page)).to be_truthy end @@ -34,13 +37,14 @@ module VCAP::CloudController let(:opts) do { names: %w(name1 name2), + default: 'true', page: 1, per_page: 5, } end it 'excludes the pagination keys' do - expected_params = [:names] + expected_params = [:names, :default] expect(StacksListMessage.from_params(opts).to_param_hash.keys).to match_array(expected_params) end end @@ -49,7 +53,8 @@ module VCAP::CloudController it 'accepts a set of fields' do expect { StacksListMessage.from_params({ - names: [] + names: [], + default: true, }) }.not_to raise_error end @@ -74,6 +79,12 @@ module VCAP::CloudController expect(message.errors[:names].length).to eq 1 end + it 'validates that default is boolean-like' do + message = StacksListMessage.from_params({ default: 'maybe' }) + expect(message).to be_invalid + expect(message.errors[:default].length).to eq 1 + end + it 'validates label selector' do message = StacksListMessage.from_params('label_selector' => '') From df8e4f77663822850a6a48f9439f789c1d675562 Mon Sep 17 00:00:00 2001 From: Greg Cobb Date: Tue, 24 Jan 2023 13:43:47 -0800 Subject: [PATCH 3/3] Extract BooleanString message validator - Error message was previously "The query parameter is invalid: Default is not included in the list" for some endpoints, which was unhelpful and NOT luxurious - Slightly modify the error message for some services endpoints --- app/messages/purge_message.rb | 2 +- app/messages/security_group_list_message.rb | 4 ++-- .../service_offerings_list_message.rb | 2 +- app/messages/service_plans_list_message.rb | 2 +- app/messages/stacks_list_message.rb | 2 +- app/messages/validators.rb | 12 ++++++++++ spec/unit/messages/purge_message_spec.rb | 2 +- .../service_offerings_list_message_spec.rb | 2 +- .../service_plans_list_message_spec.rb | 2 +- spec/unit/messages/validators_spec.rb | 22 +++++++++++++++++++ 10 files changed, 43 insertions(+), 9 deletions(-) diff --git a/app/messages/purge_message.rb b/app/messages/purge_message.rb index b852361fb28..e5076673094 100644 --- a/app/messages/purge_message.rb +++ b/app/messages/purge_message.rb @@ -7,7 +7,7 @@ class PurgeMessage < BaseMessage ] validates_with NoAdditionalParamsValidator - validates :purge, inclusion: { in: %w(true false), message: "only accepts values 'true' or 'false'" }, allow_nil: true + validates :purge, boolean_string: true, allow_nil: true def self.from_params(params) super(params, []) diff --git a/app/messages/security_group_list_message.rb b/app/messages/security_group_list_message.rb index 02b50410797..d3c311968c4 100644 --- a/app/messages/security_group_list_message.rb +++ b/app/messages/security_group_list_message.rb @@ -15,8 +15,8 @@ class SecurityGroupListMessage < ListMessage validates :names, array: true, allow_nil: true validates :running_space_guids, array: true, allow_nil: true validates :staging_space_guids, array: true, allow_nil: true - validates :globally_enabled_running, inclusion: { in: %w(true false) }, allow_nil: true - validates :globally_enabled_staging, inclusion: { in: %w(true false) }, allow_nil: true + validates :globally_enabled_running, boolean_string: true, allow_nil: true + validates :globally_enabled_staging, boolean_string: true, allow_nil: true def self.from_params(params) super(params, %w(names running_space_guids staging_space_guids)) diff --git a/app/messages/service_offerings_list_message.rb b/app/messages/service_offerings_list_message.rb index 047a3275bff..cc9734fb61c 100644 --- a/app/messages/service_offerings_list_message.rb +++ b/app/messages/service_offerings_list_message.rb @@ -19,7 +19,7 @@ class ServiceOfferingsListMessage < MetadataListMessage register_allowed_keys(@single_keys + @array_keys) validates_with NoAdditionalParamsValidator - validates :available, inclusion: { in: %w(true false), message: "only accepts values 'true' or 'false'" }, allow_nil: true + validates :available, boolean_string: true, allow_nil: true validates :fields, allow_nil: true, fields: { allowed: { diff --git a/app/messages/service_plans_list_message.rb b/app/messages/service_plans_list_message.rb index 03f94253403..bdbf6f390cb 100644 --- a/app/messages/service_plans_list_message.rb +++ b/app/messages/service_plans_list_message.rb @@ -24,7 +24,7 @@ class ServicePlansListMessage < MetadataListMessage validates_with NoAdditionalParamsValidator validates_with IncludeParamValidator, valid_values: %w(space.organization service_offering) - validates :available, inclusion: { in: %w(true false), message: "only accepts values 'true' or 'false'" }, allow_nil: true + validates :available, boolean_string: true, allow_nil: true validates :fields, allow_nil: true, fields: { allowed: { diff --git a/app/messages/stacks_list_message.rb b/app/messages/stacks_list_message.rb index 6f6515e74d7..53789cf78d2 100644 --- a/app/messages/stacks_list_message.rb +++ b/app/messages/stacks_list_message.rb @@ -7,7 +7,7 @@ class StacksListMessage < MetadataListMessage validates_with NoAdditionalParamsValidator validates :names, array: true, allow_nil: true - validates :default, inclusion: { in: %w(true false) }, allow_nil: true + validates :default, boolean_string: true, allow_nil: true def self.from_params(params) super(params, %w(names)) diff --git a/app/messages/validators.rb b/app/messages/validators.rb index 64ddbb1076e..84ba7e6b3c7 100644 --- a/app/messages/validators.rb +++ b/app/messages/validators.rb @@ -40,6 +40,18 @@ def boolean?(value) end end + class BooleanStringValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + record.errors.add attribute, message: "must be 'true' or 'false'" unless boolean?(value) + end + + private + + def boolean?(value) + ['true', 'false'].include? value + end + end + class HashValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) record.errors.add attribute, message: 'must be an object' unless value.is_a?(Hash) diff --git a/spec/unit/messages/purge_message_spec.rb b/spec/unit/messages/purge_message_spec.rb index 6e4015612bf..e1133671957 100644 --- a/spec/unit/messages/purge_message_spec.rb +++ b/spec/unit/messages/purge_message_spec.rb @@ -30,7 +30,7 @@ module VCAP::CloudController message = PurgeMessage.from_params({ purge: 'nope' }.with_indifferent_access) expect(message).not_to be_valid - expect(message.errors[:purge]).to include("only accepts values 'true' or 'false'") + expect(message.errors[:purge]).to include("must be 'true' or 'false'") end end diff --git a/spec/unit/messages/service_offerings_list_message_spec.rb b/spec/unit/messages/service_offerings_list_message_spec.rb index af9c94fdc15..7b2e4a7749f 100644 --- a/spec/unit/messages/service_offerings_list_message_spec.rb +++ b/spec/unit/messages/service_offerings_list_message_spec.rb @@ -70,7 +70,7 @@ module VCAP::CloudController message = described_class.from_params({ available: 'nope' }.with_indifferent_access) expect(message).not_to be_valid - expect(message.errors[:available]).to include("only accepts values 'true' or 'false'") + expect(message.errors[:available]).to include("must be 'true' or 'false'") end end diff --git a/spec/unit/messages/service_plans_list_message_spec.rb b/spec/unit/messages/service_plans_list_message_spec.rb index da445284f83..5cd541c0dfb 100644 --- a/spec/unit/messages/service_plans_list_message_spec.rb +++ b/spec/unit/messages/service_plans_list_message_spec.rb @@ -96,7 +96,7 @@ module VCAP::CloudController it 'does not accept other values' do message = described_class.from_params({ available: 'nope' }.with_indifferent_access) expect(message).not_to be_valid - expect(message.errors[:available]).to include("only accepts values 'true' or 'false'") + expect(message.errors[:available]).to include("must be 'true' or 'false'") end end diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 8f5e3c12b10..f804c5807fe 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -100,6 +100,28 @@ def validate_each(record, attr_name, value) end end + describe 'BooleanStringValidator' do + let(:boolean_class) do + Class.new(fake_class) do + validates :field, boolean_string: true + end + end + + it 'adds an error if the field is not a boolean string' do + instance = boolean_class.new field: 'snarf' + expect(instance.valid?).to be_falsey + expect(instance.errors[:field]).to include "must be 'true' or 'false'" + end + + it 'does not add an error if the field is a boolean string' do + instance = boolean_class.new field: 'true' + expect(instance.valid?).to be_truthy + + instance = boolean_class.new field: 'false' + expect(instance.valid?).to be_truthy + end + end + describe 'HashValidator' do let(:hash_class) do Class.new(fake_class) do