Skip to content

G
gitlab-ce
Commit 69493001 authored by Jan Provaznik's avatar Jan Provaznik
Browse files
Options

Merge branch 'lm-clean-up-jobs-refactor-ff' into 'master' 

Cleans up ci_jobs_finder_refactor ff code

See merge request gitlab-org/gitlab!45558
parents c58b6d1a 7e957445
Show whitespace changes
Inline Side-by-side
Showing with 207 additions and 612 deletions
app/finders/ci/jobs_finder.rb
View file @ 69493001
...@@ -25,11 +25,7 @@ module Ci ...@@ -25,11 +25,7 @@ module Ci
attr_reader :current_user, :pipeline, :project, :params, :type attr_reader :current_user, :pipeline, :project, :params, :type
def init_collection def init_collection
if Feature.enabled?(:ci_jobs_finder_refactor, default_enabled: true)
pipeline_jobs || project_jobs || all_jobs pipeline_jobs || project_jobs || all_jobs
else
project ? project_builds : all_jobs
end
end end
def all_jobs def all_jobs
...@@ -38,12 +34,6 @@ module Ci ...@@ -38,12 +34,6 @@ module Ci
type.all type.all
end end
def project_builds
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :read_build, project)
project.builds.relevant
end
def project_jobs def project_jobs
return unless project return unless project
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :read_build, project) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :read_build, project)
...@@ -59,9 +49,7 @@ module Ci ...@@ -59,9 +49,7 @@ module Ci
end end
def filter_by_scope(builds) def filter_by_scope(builds)
if Feature.enabled?(:ci_jobs_finder_refactor, default_enabled: true)
return filter_by_statuses!(params[:scope], builds) if params[:scope].is_a?(Array) return filter_by_statuses!(params[:scope], builds) if params[:scope].is_a?(Array)
end
case params[:scope] case params[:scope]
when 'pending' when 'pending'
......
config/feature_flags/development/ci_jobs_finder_refactor.yml deleted 100644 → 0
View file @ c58b6d1a
---
name: ci_jobs_finder_refactor
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36622
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245183
group: group::continuous integration
type: development
default_enabled: true
lib/api/ci/pipelines.rb
View file @ 69493001
...@@ -128,15 +128,9 @@ module API ...@@ -128,15 +128,9 @@ module API
pipeline = user_project.all_pipelines.find(params[:pipeline_id]) pipeline = user_project.all_pipelines.find(params[:pipeline_id])
if Feature.enabled?(:ci_jobs_finder_refactor, default_enabled: true)
builds = ::Ci::JobsFinder builds = ::Ci::JobsFinder
.new(current_user: current_user, pipeline: pipeline, params: params) .new(current_user: current_user, pipeline: pipeline, params: params)
.execute .execute
else
authorize!(:read_build, pipeline)
builds = pipeline.builds
builds = filter_builds(builds, params[:scope])
end
builds = builds.with_preloads builds = builds.with_preloads
...@@ -157,16 +151,9 @@ module API ...@@ -157,16 +151,9 @@ module API
pipeline = user_project.all_pipelines.find(params[:pipeline_id]) pipeline = user_project.all_pipelines.find(params[:pipeline_id])
if Feature.enabled?(:ci_jobs_finder_refactor, default_enabled: true)
bridges = ::Ci::JobsFinder bridges = ::Ci::JobsFinder
.new(current_user: current_user, pipeline: pipeline, params: params, type: ::Ci::Bridge) .new(current_user: current_user, pipeline: pipeline, params: params, type: ::Ci::Bridge)
.execute .execute
else
authorize!(:read_pipeline, pipeline)
bridges = pipeline.bridges
bridges = filter_builds(bridges, params[:scope])
end
bridges = bridges.with_preloads bridges = bridges.with_preloads
present paginate(bridges), with: Entities::Ci::Bridge present paginate(bridges), with: Entities::Ci::Bridge
...@@ -246,21 +233,6 @@ module API ...@@ -246,21 +233,6 @@ module API
end end
helpers do helpers do
# NOTE: This method should be removed once the ci_jobs_finder_refactor FF is
# removed. https://gitlab.com/gitlab-org/gitlab/-/issues/245183
# rubocop: disable CodeReuse/ActiveRecord
def filter_builds(builds, scope)
return builds if scope.nil? || scope.empty?
available_statuses = ::CommitStatus::AVAILABLE_STATUSES
unknown = scope - available_statuses
render_api_error!('Scope contains invalid value(s)', 400) unless unknown.empty?
builds.where(status: scope)
end
# rubocop: enable CodeReuse/ActiveRecord
def pipeline def pipeline
strong_memoize(:pipeline) do strong_memoize(:pipeline) do
user_project.all_pipelines.find(params[:pipeline_id]) user_project.all_pipelines.find(params[:pipeline_id])
......
spec/finders/ci/jobs_finder_spec.rb
View file @ 69493001
...@@ -36,11 +36,6 @@ RSpec.describe Ci::JobsFinder, '#execute' do ...@@ -36,11 +36,6 @@ RSpec.describe Ci::JobsFinder, '#execute' do
end end
end end
context 'with ci_jobs_finder_refactor ff enabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: true)
end
context 'scope is present' do context 'scope is present' do
let(:jobs) { [job_1, job_2, job_3] } let(:jobs) { [job_1, job_2, job_3] }
...@@ -69,36 +64,6 @@ RSpec.describe Ci::JobsFinder, '#execute' do ...@@ -69,36 +64,6 @@ RSpec.describe Ci::JobsFinder, '#execute' do
end end
end end
context 'with ci_jobs_finder_refactor ff disabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: false)
end
context 'scope is present' do
let(:jobs) { [job_1, job_2, job_3] }
where(:scope, :index) do
[
['pending', 0],
['running', 1],
['finished', 2]
]
end
with_them do
let(:params) { { scope: scope } }
it { expect(subject).to match_array([jobs[index]]) }
end
end
end
end
context 'with ci_jobs_finder_refactor ff enabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: true)
end
context 'a project is present' do context 'a project is present' do
subject { described_class.new(current_user: user, project: project, params: params).execute } subject { described_class.new(current_user: user, project: project, params: params).execute }
...@@ -130,42 +95,4 @@ RSpec.describe Ci::JobsFinder, '#execute' do ...@@ -130,42 +95,4 @@ RSpec.describe Ci::JobsFinder, '#execute' do
end end
end end
end end
end
context 'with ci_jobs_finder_refactor ff disabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: false)
end
context 'a project is present' do
subject { described_class.new(current_user: user, project: project, params: params).execute }
context 'user has access to the project' do
before do
project.add_maintainer(user)
end
it 'returns jobs for the specified project' do
expect(subject).to match_array([job_3])
end
end
context 'user has no access to project builds' do
before do
project.add_guest(user)
end
it 'returns no jobs' do
expect(subject).to be_empty
end
end
context 'without user' do
let(:user) { nil }
it 'returns no jobs' do
expect(subject).to be_empty
end
end
end
end
end end
spec/requests/api/ci/pipelines_spec.rb
View file @ 69493001
...@@ -325,11 +325,6 @@ RSpec.describe API::Ci::Pipelines do ...@@ -325,11 +325,6 @@ RSpec.describe API::Ci::Pipelines do
end end
end end
context 'with ci_jobs_finder_refactor ff enabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: true)
end
context 'authorized user' do context 'authorized user' do
it 'returns pipeline jobs' do it 'returns pipeline jobs' do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
...@@ -442,124 +437,6 @@ RSpec.describe API::Ci::Pipelines do ...@@ -442,124 +437,6 @@ RSpec.describe API::Ci::Pipelines do
end end
end end
context 'with ci_jobs_finder ff disabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: false)
end
context 'authorized user' do
it 'returns pipeline jobs' do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to include_pagination_headers
expect(json_response).to be_an Array
end
it 'returns correct values' do
expect(json_response).not_to be_empty
expect(json_response.first['commit']['id']).to eq project.commit.id
expect(Time.parse(json_response.first['artifacts_expire_at'])).to be_like_time(job.artifacts_expire_at)
expect(json_response.first['artifacts_file']).to be_nil
expect(json_response.first['artifacts']).to be_an Array
expect(json_response.first['artifacts']).to be_empty
end
it_behaves_like 'a job with artifacts and trace' do
let(:api_endpoint) { "/projects/#{project.id}/pipelines/#{pipeline.id}/jobs" }
end
it 'returns pipeline data' do
json_job = json_response.first
expect(json_job['pipeline']).not_to be_empty
expect(json_job['pipeline']['id']).to eq job.pipeline.id
expect(json_job['pipeline']['ref']).to eq job.pipeline.ref
expect(json_job['pipeline']['sha']).to eq job.pipeline.sha
expect(json_job['pipeline']['status']).to eq job.pipeline.status
end
context 'filter jobs with one scope element' do
let(:query) { { 'scope' => 'pending' } }
it do
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an Array
end
end
context 'filter jobs with hash' do
let(:query) { { scope: { hello: 'pending', world: 'running' } } }
it { expect(response).to have_gitlab_http_status(:bad_request) }
end
context 'filter jobs with array of scope elements' do
let(:query) { { scope: %w(pending running) } }
it do
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an Array
end
end
context 'respond 400 when scope contains invalid state' do
let(:query) { { scope: %w(unknown running) } }
it { expect(response).to have_gitlab_http_status(:bad_request) }
end
context 'jobs in different pipelines' do
let!(:pipeline2) { create(:ci_empty_pipeline, project: project) }
let!(:job2) { create(:ci_build, pipeline: pipeline2) }
it 'excludes jobs from other pipelines' do
json_response.each { |job| expect(job['pipeline']['id']).to eq(pipeline.id) }
end
end
it 'avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/jobs", api_user), params: query
end.count
create_list(:ci_build, 3, :trace_artifact, :artifacts, :test_reports, pipeline: pipeline)
expect do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/jobs", api_user), params: query
end.not_to exceed_all_query_limit(control_count)
end
end
context 'no pipeline is found' do
it 'does not return jobs' do
get api("/projects/#{project2.id}/pipelines/#{pipeline.id}/jobs", user)
expect(json_response['message']).to eq '404 Project Not Found'
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'when user is not logged in' do
let(:api_user) { nil }
it 'does not return jobs' do
expect(json_response['message']).to eq '404 Project Not Found'
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when user is guest' do
let(:guest) { create(:project_member, :guest, project: project).user }
let(:api_user) { guest }
it 'does not return jobs' do
expect(response).to have_gitlab_http_status(:forbidden)
end
end
end
end
end
describe 'GET /projects/:id/pipelines/:pipeline_id/bridges' do describe 'GET /projects/:id/pipelines/:pipeline_id/bridges' do
let_it_be(:bridge) { create(:ci_bridge, pipeline: pipeline) } let_it_be(:bridge) { create(:ci_bridge, pipeline: pipeline) }
let(:downstream_pipeline) { create(:ci_pipeline) } let(:downstream_pipeline) { create(:ci_pipeline) }
...@@ -583,11 +460,6 @@ RSpec.describe API::Ci::Pipelines do ...@@ -583,11 +460,6 @@ RSpec.describe API::Ci::Pipelines do
end end
end end
context 'with ci_jobs_finder_refactor ff enabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: true)
end
context 'authorized user' do context 'authorized user' do
it 'returns pipeline bridges' do it 'returns pipeline bridges' do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
...@@ -737,163 +609,6 @@ RSpec.describe API::Ci::Pipelines do ...@@ -737,163 +609,6 @@ RSpec.describe API::Ci::Pipelines do
end end
end end
end end
end
context 'with ci_jobs_finder_refactor ff disabled' do
before do
stub_feature_flags(ci_jobs_finder_refactor: false)
end
context 'authorized user' do
it 'returns pipeline bridges' do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to include_pagination_headers
expect(json_response).to be_an Array
end
it 'returns correct values' do
expect(json_response).not_to be_empty
expect(json_response.first['commit']['id']).to eq project.commit.id
expect(json_response.first['id']).to eq bridge.id
expect(json_response.first['name']).to eq bridge.name
expect(json_response.first['stage']).to eq bridge.stage
end
it 'returns pipeline data' do
json_bridge = json_response.first
expect(json_bridge['pipeline']).not_to be_empty
expect(json_bridge['pipeline']['id']).to eq bridge.pipeline.id
expect(json_bridge['pipeline']['ref']).to eq bridge.pipeline.ref
expect(json_bridge['pipeline']['sha']).to eq bridge.pipeline.sha
expect(json_bridge['pipeline']['status']).to eq bridge.pipeline.status
end
it 'returns downstream pipeline data' do
json_bridge = json_response.first
expect(json_bridge['downstream_pipeline']).not_to be_empty
expect(json_bridge['downstream_pipeline']['id']).to eq downstream_pipeline.id
expect(json_bridge['downstream_pipeline']['ref']).to eq downstream_pipeline.ref
expect(json_bridge['downstream_pipeline']['sha']).to eq downstream_pipeline.sha
expect(json_bridge['downstream_pipeline']['status']).to eq downstream_pipeline.status
end
context 'filter bridges' do
before_all do
create_bridge(pipeline, :pending)
create_bridge(pipeline, :running)
end
context 'with one scope element' do
let(:query) { { 'scope' => 'pending' } }
it :skip_before_request do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/bridges", api_user), params: query
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an Array
expect(json_response.count).to eq 1
expect(json_response.first["status"]).to eq "pending"
end
end
context 'with array of scope elements' do
let(:query) { { scope: %w(pending running) } }
it :skip_before_request do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/bridges", api_user), params: query
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an Array
expect(json_response.count).to eq 2
json_response.each { |r| expect(%w(pending running).include?(r['status'])).to be true }
end
end
end
context 'respond 400 when scope contains invalid state' do
context 'in an array' do
let(:query) { { scope: %w(unknown running) } }
it { expect(response).to have_gitlab_http_status(:bad_request) }
end
context 'in a hash' do
let(:query) { { scope: { unknown: true } } }
it { expect(response).to have_gitlab_http_status(:bad_request) }
end
context 'in a string' do
let(:query) { { scope: "unknown" } }
it { expect(response).to have_gitlab_http_status(:bad_request) }
end
end
context 'bridges in different pipelines' do
let!(:pipeline2) { create(:ci_empty_pipeline, project: project) }
let!(:bridge2) { create(:ci_bridge, pipeline: pipeline2) }
it 'excludes bridges from other pipelines' do
json_response.each { |bridge| expect(bridge['pipeline']['id']).to eq(pipeline.id) }
end
end
it 'avoids N+1 queries' do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/bridges", api_user), params: query
end.count
3.times { create_bridge(pipeline) }
expect do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/bridges", api_user), params: query
end.not_to exceed_all_query_limit(control_count)
end
end
context 'no pipeline is found' do
it 'does not return bridges' do
get api("/projects/#{project2.id}/pipelines/#{pipeline.id}/bridges", user)
expect(json_response['message']).to eq '404 Project Not Found'
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'when user is not logged in' do
let(:api_user) { nil }
it 'does not return bridges' do
expect(json_response['message']).to eq '404 Project Not Found'
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when user is guest' do
let(:api_user) { guest }
let(:guest) { create(:project_member, :guest, project: project).user }
it 'does not return bridges' do
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'when user has no read_build access for project' do
before do
project.add_guest(api_user)
end
it 'does not return bridges' do
get api("/projects/#{project.id}/pipelines/#{pipeline.id}/bridges", api_user)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
end
end
def create_bridge(pipeline, status = :created) def create_bridge(pipeline, status = :created)
create(:ci_bridge, status: status, pipeline: pipeline).tap do |bridge| create(:ci_bridge, status: status, pipeline: pipeline).tap do |bridge|
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7