diff options
author | Hordur Freyr Yngvason <hfyngvason@gitlab.com> | 2019-08-08 21:51:52 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2019-08-08 21:51:52 +0300 |
commit | 5f82ff1469510b4e51d531775a44e4bea92254fe (patch) | |
tree | 2762023eb50a91cabb54f8b454db49c147f447d4 /spec | |
parent | 455d16d1bfd59000391a64f41ab86d5a847f008a (diff) |
Bring scoped environment variables to core
As decided in https://gitlab.com/gitlab-org/gitlab-ce/issues/53593
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/variables_controller_spec.rb | 65 | ||||
-rw-r--r-- | spec/features/project_variables_spec.rb | 23 | ||||
-rw-r--r-- | spec/frontend/fixtures/projects.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/build/policy/variables_spec.rb | 33 | ||||
-rw-r--r-- | spec/lib/gitlab/regex_spec.rb | 8 | ||||
-rw-r--r-- | spec/models/ci/build_spec.rb | 26 | ||||
-rw-r--r-- | spec/models/ci/variable_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/concerns/has_environment_scope_spec.rb | 66 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 93 | ||||
-rw-r--r-- | spec/requests/api/variables_spec.rb | 24 | ||||
-rw-r--r-- | spec/serializers/variable_entity_spec.rb | 2 |
11 files changed, 339 insertions, 4 deletions
diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb index a2a09e2580f..21e106660d0 100644 --- a/spec/controllers/projects/variables_controller_spec.rb +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -36,5 +36,70 @@ describe Projects::VariablesController do end include_examples 'PATCH #update updates variables' + + context 'with environment scope' do + let!(:variable) { create(:ci_variable, project: project, environment_scope: 'custom_scope') } + + let(:variable_attributes) do + { id: variable.id, + key: variable.key, + secret_value: variable.value, + protected: variable.protected?.to_s, + environment_scope: variable.environment_scope } + end + let(:new_variable_attributes) do + { key: 'new_key', + secret_value: 'dummy_value', + protected: 'false', + environment_scope: 'new_scope' } + end + + context 'with same key and different environment scope' do + let(:variables_attributes) do + [ + variable_attributes, + new_variable_attributes.merge(key: variable.key) + ] + end + + it 'does not update the existing variable' do + expect { subject }.not_to change { variable.reload.value } + end + + it 'creates the new variable' do + expect { subject }.to change { owner.variables.count }.by(1) + end + + it 'returns a successful response including all variables' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('variables') + end + end + + context 'with same key and same environment scope' do + let(:variables_attributes) do + [ + variable_attributes, + new_variable_attributes.merge(key: variable.key, environment_scope: variable.environment_scope) + ] + end + + it 'does not update the existing variable' do + expect { subject }.not_to change { variable.reload.value } + end + + it 'does not create the new variable' do + expect { subject }.not_to change { owner.variables.count } + end + + it 'returns a bad request response' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + end end end diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb index 95685a3c7ff..9e3f8a843a1 100644 --- a/spec/features/project_variables_spec.rb +++ b/spec/features/project_variables_spec.rb @@ -17,4 +17,27 @@ describe 'Project variables', :js do end it_behaves_like 'variable list' + + it 'adds new variable with a special environment scope' do + page.within('.js-ci-variable-list-section .js-row:last-child') do + find('.js-ci-variable-input-key').set('somekey') + find('.js-ci-variable-input-value').set('somevalue') + + find('.js-variable-environment-toggle').click + find('.js-variable-environment-dropdown-wrapper .dropdown-input-field').set('review/*') + find('.js-variable-environment-dropdown-wrapper .js-dropdown-create-new-item').click + + expect(find('input[name="variables[variables_attributes][][environment_scope]"]', visible: false).value).to eq('review/*') + end + + click_button('Save variables') + wait_for_requests + + visit page_path + + page.within('.js-ci-variable-list-section .js-row:nth-child(2)') do + expect(find('.js-ci-variable-input-key').value).to eq('somekey') + expect(page).to have_content('review/*') + end + end end diff --git a/spec/frontend/fixtures/projects.rb b/spec/frontend/fixtures/projects.rb index b6c29003e57..91e3b65215a 100644 --- a/spec/frontend/fixtures/projects.rb +++ b/spec/frontend/fixtures/projects.rb @@ -18,8 +18,6 @@ describe 'Projects (JavaScript fixtures)', type: :controller do end before do - stub_licensed_features(variable_environment_scope: true) - project.add_maintainer(admin) sign_in(admin) allow(SecureRandom).to receive(:hex).and_return('securerandomhex:thereisnospoon') diff --git a/spec/lib/gitlab/ci/build/policy/variables_spec.rb b/spec/lib/gitlab/ci/build/policy/variables_spec.rb index 42a2a9fda2e..f712f47a558 100644 --- a/spec/lib/gitlab/ci/build/policy/variables_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/variables_spec.rb @@ -91,5 +91,38 @@ describe Gitlab::Ci::Build::Policy::Variables do expect(policy).to be_satisfied_by(pipeline, seed) end end + + context 'when using project ci variables in environment scope' do + let(:ci_build) do + build(:ci_build, pipeline: pipeline, + project: project, + ref: 'master', + stage: 'review', + environment: 'test/$CI_JOB_STAGE/1') + end + + before do + create(:ci_variable, project: project, + key: 'SCOPED_VARIABLE', + value: 'my-value-1') + + create(:ci_variable, project: project, + key: 'SCOPED_VARIABLE', + value: 'my-value-2', + environment_scope: 'test/review/*') + end + + it 'is satisfied by scoped variable match' do + policy = described_class.new(['$SCOPED_VARIABLE == "my-value-2"']) + + expect(policy).to be_satisfied_by(pipeline, seed) + end + + it 'is not satisfied when matching against overridden variable' do + policy = described_class.new(['$SCOPED_VARIABLE == "my-value-1"']) + + expect(policy).not_to be_satisfied_by(pipeline, seed) + end + end end end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index a1079e54975..ba295386a55 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -32,6 +32,14 @@ describe Gitlab::Regex do it { is_expected.not_to match('/') } end + describe '.environment_scope_regex' do + subject { described_class.environment_scope_regex } + + it { is_expected.to match('foo') } + it { is_expected.to match('foo*Z') } + it { is_expected.not_to match('!!()()') } + end + describe '.environment_slug_regex' do subject { described_class.environment_slug_regex } diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b7e005e3883..4aac4b640f4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2340,6 +2340,32 @@ describe Ci::Build do it_behaves_like 'containing environment variables' end end + + context 'when project has an environment specific variable' do + let(:environment_specific_variable) do + { key: 'MY_STAGING_ONLY_VARIABLE', value: 'environment_specific_variable', public: false, masked: false } + end + + before do + create(:ci_variable, environment_specific_variable.slice(:key, :value) + .merge(project: project, environment_scope: 'stag*')) + end + + it_behaves_like 'containing environment variables' + + context 'when environment scope does not match build environment' do + it { is_expected.not_to include(environment_specific_variable) } + end + + context 'when environment scope matches build environment' do + before do + create(:environment, name: 'staging', project: project) + build.update!(environment: 'staging') + end + + it { is_expected.to include(environment_specific_variable) } + end + end end context 'when build started manually' do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index a231c7eaed8..3ff547456c6 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -10,6 +10,7 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(Presentable) } it { is_expected.to include_module(Maskable) } + it { is_expected.to include_module(HasEnvironmentScope) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end diff --git a/spec/models/concerns/has_environment_scope_spec.rb b/spec/models/concerns/has_environment_scope_spec.rb new file mode 100644 index 00000000000..a6e1ba59263 --- /dev/null +++ b/spec/models/concerns/has_environment_scope_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HasEnvironmentScope do + subject { build(:ci_variable) } + + it { is_expected.to allow_value('*').for(:environment_scope) } + it { is_expected.to allow_value('review/*').for(:environment_scope) } + it { is_expected.not_to allow_value('').for(:environment_scope) } + it { is_expected.not_to allow_value('!!()()').for(:environment_scope) } + + it do + is_expected.to validate_uniqueness_of(:key) + .scoped_to(:project_id, :environment_scope) + .with_message(/\(\w+\) has already been taken/) + end + + describe '.on_environment' do + let(:project) { create(:project) } + + it 'returns scoped objects' do + variable1 = create(:ci_variable, project: project, environment_scope: '*') + variable2 = create(:ci_variable, project: project, environment_scope: 'product/*') + create(:ci_variable, project: project, environment_scope: 'staging/*') + + expect(project.variables.on_environment('product/canary-1')).to eq([variable1, variable2]) + end + + it 'returns only the most relevant object if relevant_only is true' do + create(:ci_variable, project: project, environment_scope: '*') + variable2 = create(:ci_variable, project: project, environment_scope: 'product/*') + create(:ci_variable, project: project, environment_scope: 'staging/*') + + expect(project.variables.on_environment('product/canary-1', relevant_only: true)).to eq([variable2]) + end + + it 'returns scopes ordered by lowest precedence first' do + create(:ci_variable, project: project, environment_scope: '*') + create(:ci_variable, project: project, environment_scope: 'production*') + create(:ci_variable, project: project, environment_scope: 'production') + + result = project.variables.on_environment('production').map(&:environment_scope) + + expect(result).to eq(['*', 'production*', 'production']) + end + end + + describe '#environment_scope=' do + context 'when the new environment_scope is nil' do + it 'strips leading and trailing whitespaces' do + subject.environment_scope = nil + + expect(subject.environment_scope).to eq('') + end + end + + context 'when the new environment_scope has leadind and trailing whitespaces' do + it 'strips leading and trailing whitespaces' do + subject.environment_scope = ' * ' + + expect(subject.environment_scope).to eq('*') + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index dde766c3813..29a589eba20 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2648,9 +2648,10 @@ describe Project do describe '#ci_variables_for' do let(:project) { create(:project) } + let(:environment_scope) { '*' } let!(:ci_variable) do - create(:ci_variable, value: 'secret', project: project) + create(:ci_variable, value: 'secret', project: project, environment_scope: environment_scope) end let!(:protected_variable) do @@ -2695,6 +2696,96 @@ describe Project do it_behaves_like 'ref is protected' end + + context 'when environment name is specified' do + let(:environment) { 'review/name' } + + subject do + project.ci_variables_for(ref: 'ref', environment: environment) + end + + context 'when environment scope is exactly matched' do + let(:environment_scope) { 'review/name' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope is matched by wildcard' do + let(:environment_scope) { 'review/*' } + + it { is_expected.to contain_exactly(ci_variable) } + end + + context 'when environment scope does not match' do + let(:environment_scope) { 'review/*/special' } + + it { is_expected.not_to contain_exactly(ci_variable) } + end + + context 'when environment scope has _' do + let(:environment_scope) { '*_*' } + + it 'does not treat it as wildcard' do + is_expected.not_to contain_exactly(ci_variable) + end + + context 'when environment name contains underscore' do + let(:environment) { 'foo_bar/test' } + let(:environment_scope) { 'foo_bar/*' } + + it 'matches literally for _' do + is_expected.to contain_exactly(ci_variable) + end + end + end + + # The environment name and scope cannot have % at the moment, + # but we're considering relaxing it and we should also make sure + # it doesn't break in case some data sneaked in somehow as we're + # not checking this integrity in database level. + context 'when environment scope has %' do + it 'does not treat it as wildcard' do + ci_variable.update_attribute(:environment_scope, '*%*') + + is_expected.not_to contain_exactly(ci_variable) + end + + context 'when environment name contains a percent' do + let(:environment) { 'foo%bar/test' } + + it 'matches literally for _' do + ci_variable.update(environment_scope: 'foo%bar/*') + + is_expected.to contain_exactly(ci_variable) + end + end + end + + context 'when variables with the same name have different environment scopes' do + let!(:partially_matched_variable) do + create(:ci_variable, + key: ci_variable.key, + value: 'partial', + environment_scope: 'review/*', + project: project) + end + + let!(:perfectly_matched_variable) do + create(:ci_variable, + key: ci_variable.key, + value: 'prefect', + environment_scope: 'review/name', + project: project) + end + + it 'puts variables matching environment scope more in the end' do + is_expected.to eq( + [ci_variable, + partially_matched_variable, + perfectly_matched_variable]) + end + end + end end describe '#any_lfs_file_locks?', :request_store do diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index 55b1419a004..69f105b71a8 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -106,6 +106,30 @@ describe API::Variables do expect(response).to have_gitlab_http_status(400) end + + it 'creates variable with a specific environment scope' do + expect do + post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', environment_scope: 'review/*' } + end.to change { project.variables.reload.count }.by(1) + + expect(response).to have_gitlab_http_status(201) + expect(json_response['key']).to eq('TEST_VARIABLE_2') + expect(json_response['value']).to eq('VALUE_2') + expect(json_response['environment_scope']).to eq('review/*') + end + + it 'allows duplicated variable key given different environment scopes' do + variable = create(:ci_variable, project: project) + + expect do + post api("/projects/#{project.id}/variables", user), params: { key: variable.key, value: 'VALUE_2', environment_scope: 'review/*' } + end.to change { project.variables.reload.count }.by(1) + + expect(response).to have_gitlab_http_status(201) + expect(json_response['key']).to eq(variable.key) + expect(json_response['value']).to eq('VALUE_2') + expect(json_response['environment_scope']).to eq('review/*') + end end context 'authorized user with invalid permissions' do diff --git a/spec/serializers/variable_entity_spec.rb b/spec/serializers/variable_entity_spec.rb index effc0022633..10664ff66ec 100644 --- a/spec/serializers/variable_entity_spec.rb +++ b/spec/serializers/variable_entity_spec.rb @@ -8,7 +8,7 @@ describe VariableEntity do subject { entity.as_json } it 'contains required fields' do - expect(subject).to include(:id, :key, :value, :protected) + expect(subject).to include(:id, :key, :value, :protected, :environment_scope) end end end |