diff options
| -rw-r--r-- | app/models/ci/trigger_request.rb | 4 | ||||
| -rw-r--r-- | app/presenters/ci/build_presenter.rb | 11 | ||||
| -rw-r--r-- | app/services/ci/create_trigger_request_service.rb | 19 | ||||
| -rw-r--r-- | app/views/projects/jobs/_sidebar.html.haml | 8 | ||||
| -rw-r--r-- | lib/api/v3/triggers.rb | 32 | ||||
| -rw-r--r-- | spec/factories/ci/builds.rb | 2 | ||||
| -rw-r--r-- | spec/factories/ci/pipeline_variables.rb (renamed from spec/factories/ci/pipeline_variable_variables.rb) | 0 | ||||
| -rw-r--r-- | spec/factories/ci/trigger_requests.rb | 9 | ||||
| -rw-r--r-- | spec/features/projects/jobs_spec.rb | 40 | ||||
| -rw-r--r-- | spec/models/ci/build_spec.rb | 28 | ||||
| -rw-r--r-- | spec/models/ci/trigger_request_spec.rb | 17 | ||||
| -rw-r--r-- | spec/presenters/ci/build_presenter_spec.rb | 34 | ||||
| -rw-r--r-- | spec/requests/api/runner_spec.rb | 31 | ||||
| -rw-r--r-- | spec/requests/api/v3/triggers_spec.rb | 5 | ||||
| -rw-r--r-- | spec/services/ci/create_trigger_request_service_spec.rb | 52 | ||||
| -rw-r--r-- | spec/views/projects/jobs/show.html.haml_spec.rb | 16 |
16 files changed, 171 insertions, 137 deletions
diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb index c58ce5c3717..2c860598281 100644 --- a/app/models/ci/trigger_request.rb +++ b/app/models/ci/trigger_request.rb @@ -6,6 +6,10 @@ module Ci belongs_to :pipeline, foreign_key: :commit_id has_many :builds + # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # Ci::TriggerRequest doesn't save variables anymore. + validates :variables, absence: true + serialize :variables # rubocop:disable Cop/ActiveRecordSerialize def user_variables diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index c495c3f39bb..255475e1fe6 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -17,5 +17,16 @@ module Ci "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" end end + + def trigger_variables + return [] unless trigger_request + + @trigger_variables ||= + if pipeline.variables.any? + pipeline.variables.map(&:to_runner_variable) + else + trigger_request.user_variables + end + end end end diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb deleted file mode 100644 index b2aa457bbd5..00000000000 --- a/app/services/ci/create_trigger_request_service.rb +++ /dev/null @@ -1,19 +0,0 @@ -# This class is deprecated because we're closing Ci::TriggerRequest. -# New class is PipelineTriggerService (app/services/ci/pipeline_trigger_service.rb) -# which is integrated with Ci::PipelineVariable instaed of Ci::TriggerRequest. -# We remove this class after we removed v1 and v3 API. This class is still being -# referred by such legacy code. -module Ci - module CreateTriggerRequestService - Result = Struct.new(:trigger_request, :pipeline) - - def self.execute(project, trigger, ref, variables = nil) - trigger_request = trigger.trigger_requests.create(variables: variables) - - pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref) - .execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request) - - Result.new(trigger_request, pipeline) - end - end -end diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml index f5d5bc7eda9..43e23bb2200 100644 --- a/app/views/projects/jobs/_sidebar.html.haml +++ b/app/views/projects/jobs/_sidebar.html.haml @@ -46,14 +46,14 @@ %span.build-light-text Token: #{@build.trigger_request.trigger.short_token} - - if @build.trigger_request.variables + - if @build.trigger_variables.any? %p %button.btn.group.btn-group-justified.reveal-variables Reveal Variables %dl.js-build-variables.trigger-build-variables.hide - - @build.trigger_request.variables.each do |key, value| - %dt.js-build-variable.trigger-build-variable= key - %dd.js-build-value.trigger-build-value= value + - @build.trigger_variables.each do |trigger_variable| + %dt.js-build-variable.trigger-build-variable= trigger_variable[:key] + %dd.js-build-value.trigger-build-value= trigger_variable[:value] %div{ class: (@build.pipeline.stages_count > 1 ? "block" : "block-last") } %p diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index e9d4c35307b..534911fde5c 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -16,25 +16,31 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do - project = find_project(params[:id]) - trigger = Ci::Trigger.find_by_token(params[:token].to_s) - not_found! unless project && trigger - unauthorized! unless trigger.project == project - # validate variables - variables = params[:variables].to_h - unless variables.all? { |key, value| key.is_a?(String) && value.is_a?(String) } + params[:variables] = params[:variables].to_h + unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } render_api_error!('variables needs to be a map of key-valued strings', 400) end - # create request and trigger builds - result = Ci::CreateTriggerRequestService.execute(project, trigger, params[:ref].to_s, variables) - pipeline = result.pipeline + project = find_project(params[:id]) + not_found! unless project + + result = Ci::PipelineTriggerService.new(project, nil, params).execute + not_found! unless result - if pipeline.persisted? - present result.trigger_request, with: ::API::V3::Entities::TriggerRequest + if result[:http_status] + render_api_error!(result[:message], result[:http_status]) else - render_validation_error!(pipeline) + pipeline = result[:pipeline] + + # We switched to Ci::PipelineVariable from Ci::TriggerRequest.variables. + # Ci::TriggerRequest doesn't save variables anymore. + # Here is copying Ci::PipelineVariable to Ci::TriggerRequest.variables for presenting the variables. + # The same endpoint in v4 API pressents Pipeline instead of TriggerRequest, so it doesn't need such a process. + trigger_request = pipeline.trigger_requests.last + trigger_request.variables = params[:variables] + + present trigger_request, with: ::API::V3::Entities::TriggerRequest end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 25ec63de94a..c2b59239af9 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -107,7 +107,7 @@ FactoryGirl.define do end trait :triggered do - trigger_request factory: :ci_trigger_request_with_variables + trigger_request factory: :ci_trigger_request end after(:build) do |build, evaluator| diff --git a/spec/factories/ci/pipeline_variable_variables.rb b/spec/factories/ci/pipeline_variables.rb index 7c1a7faec08..7c1a7faec08 100644 --- a/spec/factories/ci/pipeline_variable_variables.rb +++ b/spec/factories/ci/pipeline_variables.rb diff --git a/spec/factories/ci/trigger_requests.rb b/spec/factories/ci/trigger_requests.rb index 10e0ab4fd3c..40b8848920e 100644 --- a/spec/factories/ci/trigger_requests.rb +++ b/spec/factories/ci/trigger_requests.rb @@ -1,14 +1,5 @@ FactoryGirl.define do factory :ci_trigger_request, class: Ci::TriggerRequest do trigger factory: :ci_trigger - - factory :ci_trigger_request_with_variables do - variables do - { - TRIGGER_KEY_1: 'TRIGGER_VALUE_1', - TRIGGER_KEY_2: 'TRIGGER_VALUE_2' - } - end - end end end diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index 037ac00d39f..3b5c6966287 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -292,26 +292,44 @@ feature 'Jobs' do end feature 'Variables' do - let(:trigger_request) { create(:ci_trigger_request_with_variables) } + let(:trigger_request) { create(:ci_trigger_request) } let(:job) do create :ci_build, pipeline: pipeline, trigger_request: trigger_request end - before do - visit project_job_path(project, job) + shared_examples 'expected variables behavior' do + it 'shows variable key and value after click', js: true do + expect(page).to have_css('.reveal-variables') + expect(page).not_to have_css('.js-build-variable') + expect(page).not_to have_css('.js-build-value') + + click_button 'Reveal Variables' + + expect(page).not_to have_css('.reveal-variables') + expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') + expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + end end - it 'shows variable key and value after click', js: true do - expect(page).to have_css('.reveal-variables') - expect(page).not_to have_css('.js-build-variable') - expect(page).not_to have_css('.js-build-value') + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) - click_button 'Reveal Variables' + visit project_job_path(project, job) + end + + it_behaves_like 'expected variables behavior' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + + visit project_job_path(project, job) + end - expect(page).not_to have_css('.reveal-variables') - expect(page).to have_selector('.js-build-variable', text: 'TRIGGER_KEY_1') - expect(page).to have_selector('.js-build-value', text: 'TRIGGER_VALUE_1') + it_behaves_like 'expected variables behavior' end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 3fe3ec17d36..08d22f166e4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1492,10 +1492,12 @@ describe Ci::Build do context 'when build is for triggers' do let(:trigger) { create(:ci_trigger, project: project) } - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline, trigger: trigger) } + let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } + let(:user_trigger_variable) do - { key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1', public: false } + { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false } end + let(:predefined_trigger_variable) do { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true } end @@ -1504,8 +1506,26 @@ describe Ci::Build do build.trigger_request = trigger_request end - it { is_expected.to include(user_trigger_variable) } - it { is_expected.to include(predefined_trigger_variable) } + shared_examples 'returns variables for triggers' do + it { is_expected.to include(user_trigger_variable) } + it { is_expected.to include(predefined_trigger_variable) } + end + + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + end + + it_behaves_like 'returns variables for triggers' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') + end + + it_behaves_like 'returns variables for triggers' + end end context 'when pipeline has a variable' do diff --git a/spec/models/ci/trigger_request_spec.rb b/spec/models/ci/trigger_request_spec.rb new file mode 100644 index 00000000000..7dcf3528f73 --- /dev/null +++ b/spec/models/ci/trigger_request_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe Ci::TriggerRequest do + describe 'validation' do + it 'be invalid if saving a variable' do + trigger = build(:ci_trigger_request, variables: { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } ) + + expect(trigger).not_to be_valid + end + + it 'be valid if not saving a variable' do + trigger = build(:ci_trigger_request) + + expect(trigger).to be_valid + end + end +end diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index a7a34ecac72..1a8001be6ab 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -100,4 +100,38 @@ describe Ci::BuildPresenter do end end end + + describe '#trigger_variables' do + let(:build) { create(:ci_build, pipeline: pipeline, trigger_request: trigger_request) } + let(:trigger) { create(:ci_trigger, project: project) } + let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } + + context 'when variable is stored in ci_pipeline_variables' do + let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } + + context 'when pipeline is triggered by trigger API' do + it 'returns variables' do + expect(presenter.trigger_variables).to eq([pipeline_variable.to_runner_variable]) + end + end + + context 'when pipeline is not triggered by trigger API' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'does not return variables' do + expect(presenter.trigger_variables).to eq([]) + end + end + end + + context 'when variable is stored in ci_trigger_requests.variables' do + before do + trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) + end + + it 'returns variables' do + expect(presenter.trigger_variables).to eq(trigger_request.user_variables) + end + end + end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 419a65c69b5..12720355a6d 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -557,17 +557,36 @@ describe API::Runner do { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] end + let(:trigger) { create(:ci_trigger, project: project) } + let!(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, builds: [job], trigger: trigger) } + before do - trigger = create(:ci_trigger, project: project) - create(:ci_trigger_request_with_variables, pipeline: pipeline, builds: [job], trigger: trigger) project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value') end - it 'returns variables for triggers' do - request_job + shared_examples 'expected variables behavior' do + it 'returns variables for triggers' do + request_job - expect(response).to have_http_status(201) - expect(json_response['variables']).to include(*expected_variables) + expect(response).to have_http_status(201) + expect(json_response['variables']).to include(*expected_variables) + end + end + + context 'when variables are stored in trigger_request' do + before do + trigger_request.update_attribute(:variables, { TRIGGER_KEY_1: 'TRIGGER_VALUE_1' } ) + end + + it_behaves_like 'expected variables behavior' + end + + context 'when variables are stored in pipeline_variables' do + before do + create(:ci_pipeline_variable, pipeline: pipeline, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1') + end + + it_behaves_like 'expected variables behavior' end end diff --git a/spec/requests/api/v3/triggers_spec.rb b/spec/requests/api/v3/triggers_spec.rb index d4648136841..7ccf387f2dc 100644 --- a/spec/requests/api/v3/triggers_spec.rb +++ b/spec/requests/api/v3/triggers_spec.rb @@ -37,7 +37,7 @@ describe API::V3::Triggers do it 'returns unauthorized if token is for different project' do post v3_api("/projects/#{project2.id}/trigger/builds"), options.merge(ref: 'master') - expect(response).to have_http_status(401) + expect(response).to have_http_status(404) end end @@ -80,7 +80,8 @@ describe API::V3::Triggers do post v3_api("/projects/#{project.id}/trigger/builds"), options.merge(variables: variables, ref: 'master') expect(response).to have_http_status(201) pipeline.builds.reload - expect(pipeline.builds.first.trigger_request.variables).to eq(variables) + expect(pipeline.variables.map { |v| { v.key => v.value } }.first).to eq(variables) + expect(json_response['variables']).to eq(variables) end end end diff --git a/spec/services/ci/create_trigger_request_service_spec.rb b/spec/services/ci/create_trigger_request_service_spec.rb deleted file mode 100644 index 8295813a1ca..00000000000 --- a/spec/services/ci/create_trigger_request_service_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -require 'spec_helper' - -describe Ci::CreateTriggerRequestService do - let(:service) { described_class } - let(:project) { create(:project, :repository) } - let(:trigger) { create(:ci_trigger, project: project, owner: owner) } - let(:owner) { create(:user) } - - before do - stub_ci_pipeline_to_return_yaml_file - - project.add_developer(owner) - end - - describe '#execute' do - context 'valid params' do - subject { service.execute(project, trigger, 'master') } - - context 'without owner' do - it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } - it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } - it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } - it { expect(subject.pipeline).to be_trigger } - end - - context 'with owner' do - it { expect(subject.trigger_request).to be_kind_of(Ci::TriggerRequest) } - it { expect(subject.trigger_request.builds.first).to be_kind_of(Ci::Build) } - it { expect(subject.trigger_request.builds.first.user).to eq(owner) } - it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } - it { expect(subject.pipeline).to be_trigger } - it { expect(subject.pipeline.user).to eq(owner) } - end - end - - context 'no commit for ref' do - subject { service.execute(project, trigger, 'other-branch') } - - it { expect(subject.pipeline).not_to be_persisted } - end - - context 'no builds created' do - subject { service.execute(project, trigger, 'master') } - - before do - stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }') - end - - it { expect(subject.pipeline).not_to be_persisted } - end - end -end diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb index 117f48450e2..d4279626e75 100644 --- a/spec/views/projects/jobs/show.html.haml_spec.rb +++ b/spec/views/projects/jobs/show.html.haml_spec.rb @@ -195,20 +195,4 @@ describe 'projects/jobs/show' do text: /\A\n#{Regexp.escape(commit_title)}\n\Z/) end end - - describe 'shows trigger variables in sidebar' do - let(:trigger_request) { create(:ci_trigger_request_with_variables, pipeline: pipeline) } - - before do - build.trigger_request = trigger_request - render - end - - it 'shows trigger variables in separate lines' do - expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_1') - expect(rendered).to have_css('.js-build-variable', visible: false, text: 'TRIGGER_KEY_2') - expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_1') - expect(rendered).to have_css('.js-build-value', visible: false, text: 'TRIGGER_VALUE_2') - end - end end |