Read about our upcoming Code of Conduct on this issue

Commit b3e10444 authored by Steve Azzopardi's avatar Steve Azzopardi
Browse files

Refactor project.latest_successful_builds_for def

`project.latest_successful_builds_for(ref)` is being used to find a
single job all the time. This results into us having to call `find_by`
inside of the controller which violates our CodeReuse/ActiveRecord
rubocop rule.

Refactor `project.latest_successful_builds_for(ref)` to
`project.latest_successful_build_for(job_name, ref)` which will execute
the `find_by` inside of the model.

Also create `project.latest_successful_build_for!(job_name, ref)` which
raises an exception instead of returning nil.
parent 23d9d3c1002b
...@@ -91,14 +91,11 @@ def build_from_id ...@@ -91,14 +91,11 @@ def build_from_id
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def build_from_ref def build_from_ref
return unless @ref_name return unless @ref_name
builds = project.latest_successful_builds_for(@ref_name) project.latest_successful_build_for(params[:job], @ref_name)
builds.find_by(name: params[:job])
end end
# rubocop: enable CodeReuse/ActiveRecord
def artifacts_file def artifacts_file
@artifacts_file ||= build&.artifacts_file_for_type(params[:file_type] || :archive) @artifacts_file ||= build&.artifacts_file_for_type(params[:file_type] || :archive)
......
...@@ -50,12 +50,9 @@ def job_from_id ...@@ -50,12 +50,9 @@ def job_from_id
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def job_from_ref def job_from_ref
return unless @ref_name return unless @ref_name
jobs = project.latest_successful_builds_for(@ref_name) project.latest_successful_build_for(params[:job], @ref_name)
jobs.find_by(name: params[:job])
end end
# rubocop: enable CodeReuse/ActiveRecord
end end
...@@ -644,19 +644,15 @@ def commits_by(oids:) ...@@ -644,19 +644,15 @@ def commits_by(oids:)
end end
# ref can't be HEAD, can only be branch/tag name or SHA # ref can't be HEAD, can only be branch/tag name or SHA
def latest_successful_builds_for(ref = default_branch) def latest_successful_build_for(job_name, ref = default_branch)
latest_pipeline = ci_pipelines.latest_successful_for(ref) latest_pipeline = ci_pipelines.latest_successful_for(ref)
return unless latest_pipeline
if latest_pipeline latest_pipeline.builds.latest.with_artifacts_archive.find_by(name: job_name)
latest_pipeline.builds.latest.with_artifacts_archive
else
builds.none
end
end end
def latest_successful_build_for(job_name, ref = default_branch) def latest_successful_build_for!(job_name, ref = default_branch)
builds = latest_successful_builds_for(ref) latest_successful_build_for(job_name, ref) or raise ActiveRecord::RecordNotFound.new("Couldn't find job #{job_name}", job_name)
builds.find_by!(name: job_name)
end end
def merge_base_commit(first_commit_id, second_commit_id) def merge_base_commit(first_commit_id, second_commit_id)
......
...@@ -23,17 +23,14 @@ def authorize_download_artifacts! ...@@ -23,17 +23,14 @@ def authorize_download_artifacts!
requires :job, type: String, desc: 'The name for the job' requires :job, type: String, desc: 'The name for the job'
end end
route_setting :authentication, job_token_allowed: true route_setting :authentication, job_token_allowed: true
# rubocop: disable CodeReuse/ActiveRecord
get ':id/jobs/artifacts/:ref_name/download', get ':id/jobs/artifacts/:ref_name/download',
requirements: { ref_name: /.+/ } do requirements: { ref_name: /.+/ } do
authorize_download_artifacts! authorize_download_artifacts!
builds = user_project.latest_successful_builds_for(params[:ref_name]) latest_build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
latest_build = builds.find_by!(name: params[:job])
present_carrierwave_file!(latest_build.artifacts_file) present_carrierwave_file!(latest_build.artifacts_file)
end end
# rubocop: enable CodeReuse/ActiveRecord
desc 'Download a specific file from artifacts archive from a ref' do desc 'Download a specific file from artifacts archive from a ref' do
detail 'This feature was introduced in GitLab 11.5' detail 'This feature was introduced in GitLab 11.5'
...@@ -48,7 +45,7 @@ def authorize_download_artifacts! ...@@ -48,7 +45,7 @@ def authorize_download_artifacts!
requirements: { ref_name: /.+/ } do requirements: { ref_name: /.+/ } do
authorize_download_artifacts! authorize_download_artifacts!
build = user_project.latest_successful_build_for(params[:job], params[:ref_name]) build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
path = Gitlab::Ci::Build::Artifacts::Path path = Gitlab::Ci::Build::Artifacts::Path
.new(params[:artifact_path]) .new(params[:artifact_path])
......
...@@ -1921,7 +1921,7 @@ def create_pipeline ...@@ -1921,7 +1921,7 @@ def create_pipeline
end end
end end
describe '#latest_successful_builds_for and #latest_successful_build_for' do describe '#latest_successful_build_for' do
def create_pipeline(status = 'success') def create_pipeline(status = 'success')
create(:ci_pipeline, project: project, create(:ci_pipeline, project: project,
sha: project.commit.sha, sha: project.commit.sha,
...@@ -1946,12 +1946,10 @@ def create_build(new_pipeline = pipeline, name = 'test') ...@@ -1946,12 +1946,10 @@ def create_build(new_pipeline = pipeline, name = 'test')
create_build(pipeline1, 'test') create_build(pipeline1, 'test')
create_build(pipeline1, 'test2') create_build(pipeline1, 'test2')
build1_p2 = create_build(pipeline2, 'test') build1_p2 = create_build(pipeline2, 'test')
build2_p2 = create_build(pipeline2, 'test2') create_build(pipeline2, 'test2')
latest_builds = project.latest_successful_builds_for
single_build = project.latest_successful_build_for(build1_p2.name) single_build = project.latest_successful_build_for(build1_p2.name)
expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
expect(single_build).to eq(build1_p2) expect(single_build).to eq(build1_p2)
end end
end end
...@@ -1961,22 +1959,19 @@ def create_build(new_pipeline = pipeline, name = 'test') ...@@ -1961,22 +1959,19 @@ def create_build(new_pipeline = pipeline, name = 'test')
context 'standalone pipeline' do context 'standalone pipeline' do
it 'returns builds for ref for default_branch' do it 'returns builds for ref for default_branch' do
builds = project.latest_successful_builds_for
single_build = project.latest_successful_build_for(build.name) single_build = project.latest_successful_build_for(build.name)
expect(builds).to contain_exactly(build)
expect(single_build).to eq(build) expect(single_build).to eq(build)
end end
it 'returns empty relation if the build cannot be found for #latest_successful_builds_for' do it 'returns empty relation if the build cannot be found for #latest_successful_build_for' do
builds = project.latest_successful_builds_for('TAIL') builds = project.latest_successful_build_for('TAIL')
expect(builds).to be_kind_of(ActiveRecord::Relation) expect(builds).to be_nil
expect(builds).to be_empty
end end
it 'returns exception if the build cannot be found for #latest_successful_build_for' do it 'returns exception if the build cannot be found for #latest_successful_build_for!' do
expect { project.latest_successful_build_for(build.name, 'TAIL') }.to raise_error(ActiveRecord::RecordNotFound) expect { project.latest_successful_build_for!(build.name, 'TAIL') }.to raise_error(ActiveRecord::RecordNotFound)
end end
end end
...@@ -1986,26 +1981,21 @@ def create_build(new_pipeline = pipeline, name = 'test') ...@@ -1986,26 +1981,21 @@ def create_build(new_pipeline = pipeline, name = 'test')
end end
it 'gives the latest build from latest pipeline' do it 'gives the latest build from latest pipeline' do
latest_builds = project.latest_successful_builds_for
last_single_build = project.latest_successful_build_for(build.name) last_single_build = project.latest_successful_build_for(build.name)
expect(latest_builds).to contain_exactly(build)
expect(last_single_build).to eq(build) expect(last_single_build).to eq(build)
end end
end end
end end
context 'with pending pipeline' do context 'with pending pipeline' do
before do it 'returns empty relation' do
pipeline.update(status: 'pending') pipeline.update(status: 'pending')
create_build(pipeline) pending_build = create_build(pipeline)
end
it 'returns empty relation' do expect(project.latest_successful_build_for(pending_build.name)).to be_nil
builds = project.latest_successful_builds_for
expect(builds).to be_kind_of(ActiveRecord::Relation) expect { project.latest_successful_build_for!(pending_build.name) }.to raise_error(ActiveRecord::RecordNotFound)
expect(builds).to be_empty
end end
end end
end end
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment