From ab1b173f3a36d5414226bf6ce8fab48dec7a2e14 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 2 May 2017 17:44:14 +0200 Subject: Create static model Ci::Group Now a new model is introduced, which is not persisted in the database, Ci::Group. Given a pipeline now has many stages, which in turn have many groups, we can clean up some code. Other than that, some misc changes are included to clean up. --- app/controllers/projects/pipelines_controller.rb | 6 +-- app/models/ci/group.rb | 26 +++++++++++ app/models/ci/stage.rb | 8 ++++ app/serializers/base_serializer.rb | 2 - app/serializers/job_group_entity.rb | 15 ++++++ app/serializers/jobs_serializer.rb | 53 ---------------------- app/serializers/pipeline_entity.rb | 3 +- app/serializers/stage_entity.rb | 5 +- app/services/ci/expire_pipeline_cache_service.rb | 28 +++++++----- .../projects/pipelines_controller_spec.rb | 3 +- spec/models/ci/group_spec.rb | 19 ++++++++ .../ci/expire_pipeline_cache_service_spec.rb | 28 ++++++++++++ 12 files changed, 118 insertions(+), 78 deletions(-) create mode 100644 app/models/ci/group.rb create mode 100644 app/serializers/job_group_entity.rb delete mode 100644 app/serializers/jobs_serializer.rb create mode 100644 spec/models/ci/group_spec.rb create mode 100644 spec/services/ci/expire_pipeline_cache_service_spec.rb diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index ea2915a4de6..7bed6707fe8 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -73,9 +73,9 @@ class Projects::PipelinesController < Projects::ApplicationController format.json do Gitlab::PollingInterval.set_header(response, interval: POLLING_INTERVAL) - render json: PipelineSerializer. - new(project: @project, user: @current_user). - represent(@pipeline) + render json: PipelineSerializer + .new(project: @project, user: @current_user) + .represent(@pipeline, with_jobs: true) end end end diff --git a/app/models/ci/group.rb b/app/models/ci/group.rb new file mode 100644 index 00000000000..d7a499f3368 --- /dev/null +++ b/app/models/ci/group.rb @@ -0,0 +1,26 @@ +module Ci + # Currently this model is not persisted in the database, but derived from a + # pipelines jobs. We might, but at the same time might not, persist this model + # in the database later + class Group + include StaticModel + + attr_reader :stage, :name, :statuses + + def initialize(stage, name:, statuses:) + @stage = stage + @name = name + @statuses = statuses + end + + def detailed_status(current_user) + Gitlab::Ci::Status::Group::Factory + .new(CommitStatus.where(id: statuses.map(&:id)), current_user) + .fabricate! + end + + def size + statuses.size + end + end +end diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index e7d6b17d445..9f4f9f26b1c 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -15,6 +15,14 @@ module Ci @warnings = warnings end + def job_groups + @job_groups ||= + statuses.sort_by(&:sortable_name).group_by(&:group_name) + .map do |group_name, grouped_statuses| + Ci::Group.new(self, name: group_name, statuses: grouped_statuses) + end + end + def to_param name end diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb index 37e5a7bf168..f262b7b7eb0 100644 --- a/app/serializers/base_serializer.rb +++ b/app/serializers/base_serializer.rb @@ -1,6 +1,4 @@ class BaseSerializer - attr_reader :request - def initialize(parameters = {}) @request = EntityRequest.new(parameters) end diff --git a/app/serializers/job_group_entity.rb b/app/serializers/job_group_entity.rb new file mode 100644 index 00000000000..e2e302d0bff --- /dev/null +++ b/app/serializers/job_group_entity.rb @@ -0,0 +1,15 @@ +class JobGroupEntity < Grape::Entity + include RequestAwareEntity + + expose :name + expose :size + expose :detailed_status, as: :status, with: StatusEntity + + private + + alias_method :group, :object + + def detailed_status + group.detailed_status(request.user) + end +end diff --git a/app/serializers/jobs_serializer.rb b/app/serializers/jobs_serializer.rb deleted file mode 100644 index 00a0672fda6..00000000000 --- a/app/serializers/jobs_serializer.rb +++ /dev/null @@ -1,53 +0,0 @@ -class JobsSerializer < BaseSerializer - Item = Struct.new(:name, :size, :list) - - entity BuildEntity - - def with_groups - tap { @groups = true } - end - - def groups? - @groups - end - - def represent(resource, opts = {}) - if groups? - groups(resource).map do |item| - { name: item.name, - size: item.size, - list: super(item.list, opts), - status: represent_status(item.list) } - end - else - super(resource, opts) - end - end - - private - - def represent_status(list, opts = {}) - # TODO: We don't really have a first class concept - # for JobsGroup that would make it possible to have status for that - detailed_status = - if group_jobs.one? - group_jobs.first.detailed_status(request.user) - else - Gitlab::Ci::Status::Group::Factory - .new(CommitStatus.where(id: group_jobs), request.user) - .fabricate! - end - - StatusEntity - .represent(resource, opts.merge(request: @request)) - .as_json - end - - def groups(resource) - items = resource.sort_by(&:sortable_name).group_by(&:group_name) - - items.map do |group_name, group_jobs| - Item.new(group_name, group_jobs.size, group_jobs) - end - end -end diff --git a/app/serializers/pipeline_entity.rb b/app/serializers/pipeline_entity.rb index d221b09d9cb..9c91ab3e098 100644 --- a/app/serializers/pipeline_entity.rb +++ b/app/serializers/pipeline_entity.rb @@ -11,12 +11,11 @@ class PipelineEntity < Grape::Entity pipeline) end - expose :stages, with: StageEntity - expose :details do expose :detailed_status, as: :status, with: StatusEntity expose :duration expose :finished_at + expose :stages, with: StageEntity expose :artifacts, using: BuildArtifactEntity expose :manual_actions, using: BuildActionEntity end diff --git a/app/serializers/stage_entity.rb b/app/serializers/stage_entity.rb index 35245ae978e..5b0877d53fa 100644 --- a/app/serializers/stage_entity.rb +++ b/app/serializers/stage_entity.rb @@ -26,10 +26,7 @@ class StageEntity < Grape::Entity format: :json) end - expose :jobs do |stage| - JobsSerializer.new(project: request.project, user: request.user) - .with_groups.represent(stage.statuses) - end + expose :job_groups, as: :groups, with: JobGroupEntity private diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb index ad968085aa7..6a7ac6dab35 100644 --- a/app/services/ci/expire_pipeline_cache_service.rb +++ b/app/services/ci/expire_pipeline_cache_service.rb @@ -1,31 +1,35 @@ module Ci class ExpirePipelineCacheService < BaseService + include Gitlab::Routing.url_helpers + attr_reader :pipeline def execute(pipeline) @pipeline = pipeline - store = Gitlab::EtagCaching::Store.new - store.touch(project_pipeline_path) - store.touch(project_pipelines_path) - store.touch(commit_pipelines_path) if pipeline.commit - store.touch(new_merge_request_pipelines_path) - merge_requests_pipelines_paths.each { |path| store.touch(path) } + Gitlab::EtagCaching::Store.new.tap do |store| + store.touch(project_pipeline_path) + store.touch(project_pipelines_path) + store.touch(commit_pipelines_path) if pipeline.commit + store.touch(new_merge_request_pipelines_path) + + merge_requests_pipelines_paths.each { |path| store.touch(path) } + end - Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(@pipeline) + Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline) end private def project_pipelines_path - Gitlab::Routing.url_helpers.namespace_project_pipelines_path( + namespace_project_pipelines_path( project.namespace, project, format: :json) end def commit_pipelines_path - Gitlab::Routing.url_helpers.pipelines_namespace_project_commit_path( + pipelines_namespace_project_commit_path( project.namespace, project, pipeline.commit.id, @@ -33,7 +37,7 @@ module Ci end def new_merge_request_pipelines_path - Gitlab::Routing.url_helpers.new_namespace_project_merge_request_path( + new_namespace_project_merge_request_path( project.namespace, project, format: :json) @@ -41,7 +45,7 @@ module Ci def merge_requests_pipelines_paths pipeline.merge_requests.collect do |merge_request| - Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path( + pipelines_namespace_project_merge_request_path( project.namespace, project, merge_request, @@ -50,7 +54,7 @@ module Ci end def project_pipeline_path - Gitlab::Routing.url_helpers.namespace_project_pipeline_path( + namespace_project_pipeline_path( project.namespace, project, pipeline, diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 670052f079a..f54665ebd19 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -26,6 +26,7 @@ describe Projects::PipelinesController do it 'returns JSON with serialized pipelines' do expect(response).to have_http_status(:ok) + expect(response).to match_response_schema('pipeline') expect(json_response).to include('pipelines') expect(json_response['pipelines'].count).to eq 4 @@ -33,7 +34,6 @@ describe Projects::PipelinesController do expect(json_response['count']['running']).to eq 1 expect(json_response['count']['pending']).to eq 1 expect(json_response['count']['finished']).to eq 1 - expect(json_response).not_to have_key('stages') end end @@ -46,7 +46,6 @@ describe Projects::PipelinesController do expect(response).to have_http_status(:ok) expect(json_response).not_to be_an(Array) expect(json_response['id']).to be(pipeline.id) - expect(json_response).to have_key('stages') end end diff --git a/spec/models/ci/group_spec.rb b/spec/models/ci/group_spec.rb new file mode 100644 index 00000000000..ffa3e97ec3e --- /dev/null +++ b/spec/models/ci/group_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe Ci::Group, models: true do + subject { described_class.new('test', name: 'rspec', statuses: []) } + + describe 'expectations' do + it { is_expected.to include_module(StaticModel) } + + it { is_expected.to respond_to(:stage) } + it { is_expected.to respond_to(:name) } + it { is_expected.to respond_to(:statuses) } + end + + describe '#size' do + it 'returns the size of the statusses array' do + expect(subject.size).to eq(0) + end + end +end diff --git a/spec/services/ci/expire_pipeline_cache_service_spec.rb b/spec/services/ci/expire_pipeline_cache_service_spec.rb new file mode 100644 index 00000000000..94f2352a7d6 --- /dev/null +++ b/spec/services/ci/expire_pipeline_cache_service_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe Ci::ExpirePipelineCacheService, services: true do + let(:project) { pipeline.project } + let(:pipeline) { create(:ci_pipeline) } + + before do + stub_ci_pipeline_to_return_yaml_file + end + + subject { described_class.new(project, nil).execute(pipeline) } + + describe '#execute' do + it 'creates a new Store' do + expect(Gitlab::EtagCaching::Store).to receive(:new) + .and_call_original + + subject + end + + it 'updates the ProjectPipelineStatus cache' do + expect(Gitlab::Cache::Ci::ProjectPipelineStatus) + .to receive(:update_for_pipeline).with(pipeline) + + subject + end + end +end -- cgit v1.2.1