Merge branch 'fix/sm/35650-remove-createtriggerrequestservice-and-forbid-to-persist-variables-on-ci-triggerrequest' into 'master'
Removes `CreateTriggerRequestService` and add a blocker to prevent saving variables on `Ci::TriggerRequest` Closes #35650 See merge request !13792
This commit is contained in:
commit
3d61421fb2
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
||||
if pipeline.persisted?
|
||||
present result.trigger_request, with: ::API::V3::Entities::TriggerRequest
|
||||
result = Ci::PipelineTriggerService.new(project, nil, params).execute
|
||||
not_found! unless result
|
||||
|
||||
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
|
||||
|
||||
|
|
|
@ -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|
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
||||
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
|
||||
|
||||
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
|
||||
|
||||
it_behaves_like 'expected variables behavior'
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue