From 0cfa25ff21322d45aaef91fc7a3dd7c570632a05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 16 Apr 2019 13:06:52 +0200 Subject: [PATCH] Backport changes from EE This backports the changes from https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10452 --- .../projects/merge_requests_controller.rb | 30 ++++++++++--------- app/models/ci/build.rb | 4 +-- app/models/ci/job_artifact.rb | 7 +++-- app/models/ci/pipeline.rb | 10 +++++-- app/models/merge_request.rb | 2 +- lib/gitlab/ci/config/entry/reports.rb | 3 +- spec/models/ci/build_spec.rb | 4 +-- spec/models/ci/pipeline_spec.rb | 24 +++++++++++++-- spec/services/ci/retry_build_service_spec.rb | 2 +- 9 files changed, 58 insertions(+), 28 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 39ba2a651d4..8f177895b08 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -98,20 +98,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def test_reports - result = @merge_request.compare_test_reports - - case result[:status] - when :parsing - Gitlab::PollingInterval.set_header(response, interval: 3000) - - render json: '', status: :no_content - when :parsed - render json: result[:data].to_json, status: :ok - when :error - render json: { status_reason: result[:status_reason] }, status: :bad_request - else - render json: { status_reason: 'Unknown error' }, status: :internal_server_error - end + reports_response(@merge_request.compare_test_reports) end def edit @@ -353,4 +340,19 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42441 Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42438') end + + def reports_response(report_comparison) + case report_comparison[:status] + when :parsing + ::Gitlab::PollingInterval.set_header(response, interval: 3000) + + render json: '', status: :no_content + when :parsed + render json: report_comparison[:data].to_json, status: :ok + when :error + render json: { status_reason: report_comparison[:status_reason] }, status: :bad_request + else + render json: { status_reason: 'Unknown error' }, status: :internal_server_error + end + end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index b8a76e662b0..5cf9bb4979a 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -104,8 +104,8 @@ module Ci where('NOT EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').trace) end - scope :with_test_reports, ->() do - with_existing_job_artifacts(Ci::JobArtifact.test_reports) + scope :with_reports, ->(reports_scope) do + with_existing_job_artifacts(reports_scope) .eager_load_job_artifacts end diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 99512a7c1dd..9695d49d18b 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -21,7 +21,8 @@ module Ci container_scanning: 'gl-container-scanning-report.json', dast: 'gl-dast-report.json', license_management: 'gl-license-management-report.json', - performance: 'performance.json' + performance: 'performance.json', + metrics: 'metrics.txt' }.freeze TYPE_AND_FORMAT_PAIRS = { @@ -29,6 +30,7 @@ module Ci metadata: :gzip, trace: :raw, junit: :gzip, + metrics: :gzip, # All these file formats use `raw` as we need to store them uncompressed # for Frontend to fetch the files and do analysis @@ -88,7 +90,8 @@ module Ci dast: 8, ## EE-specific codequality: 9, ## EE-specific license_management: 10, ## EE-specific - performance: 11 ## EE-specific + performance: 11, ## EE-specific + metrics: 12 ## EE-specific } enum file_format: { diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index b81a3cf8362..16f14fe9408 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -210,6 +210,10 @@ module Ci where(source: branch_pipeline_sources).where(ref: ref, tag: false) end + scope :with_reports, -> (reports_scope) do + where('EXISTS (?)', ::Ci::Build.latest.with_reports(reports_scope).where('ci_pipelines.id=ci_builds.commit_id').select(1)) + end + # Returns the pipelines in descending order (= newest first), optionally # limited to a number of references. # @@ -689,13 +693,13 @@ module Ci @latest_builds_with_artifacts ||= builds.latest.with_artifacts_archive.to_a end - def has_test_reports? - complete? && builds.latest.with_test_reports.any? + def has_reports?(reports_scope) + complete? && builds.latest.with_reports(reports_scope).exists? end def test_reports Gitlab::Ci::Reports::TestReports.new.tap do |test_reports| - builds.latest.with_test_reports.each do |build| + builds.latest.with_reports(Ci::JobArtifact.test_reports).each do |build| build.collect_test_reports!(test_reports) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0a39a720766..251a7ff41f5 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1152,7 +1152,7 @@ class MergeRequest < ApplicationRecord end def has_test_reports? - actual_head_pipeline&.has_test_reports? + actual_head_pipeline&.has_reports?(Ci::JobArtifact.test_reports) end def predefined_variables diff --git a/lib/gitlab/ci/config/entry/reports.rb b/lib/gitlab/ci/config/entry/reports.rb index a3f6cc31321..eeaac52eefd 100644 --- a/lib/gitlab/ci/config/entry/reports.rb +++ b/lib/gitlab/ci/config/entry/reports.rb @@ -11,7 +11,7 @@ module Gitlab include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Attributable - ALLOWED_KEYS = %i[junit codequality sast dependency_scanning container_scanning dast performance license_management].freeze + ALLOWED_KEYS = %i[junit codequality sast dependency_scanning container_scanning dast performance license_management metrics].freeze attributes ALLOWED_KEYS @@ -28,6 +28,7 @@ module Gitlab validates :dast, array_of_strings_or_string: true validates :performance, array_of_strings_or_string: true validates :license_management, array_of_strings_or_string: true + validates :metrics, array_of_strings_or_string: true end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1352a2de2d7..66be192ab21 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -166,8 +166,8 @@ describe Ci::Build do end end - describe '.with_test_reports' do - subject { described_class.with_test_reports } + describe '.with_reports' do + subject { described_class.with_reports(Ci::JobArtifact.test_reports) } context 'when build has a test report' do let!(:build) { create(:ci_build, :success, :test_reports) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index f3e78630c1b..f8d7579ab6a 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -426,6 +426,26 @@ describe Ci::Pipeline, :mailer do end end + describe '.with_reports' do + subject { described_class.with_reports(Ci::JobArtifact.test_reports) } + + context 'when pipeline has a test report' do + let!(:pipeline_with_report) { create(:ci_pipeline, :with_test_reports) } + + it 'selects the pipeline' do + is_expected.to eq([pipeline_with_report]) + end + end + + context 'when pipeline does not have metrics reports' do + let!(:pipeline_without_report) { create(:ci_empty_pipeline) } + + it 'does not select the pipeline' do + is_expected.to be_empty + end + end + end + describe '.merge_request_event' do subject { described_class.merge_request_event } @@ -2728,8 +2748,8 @@ describe Ci::Pipeline, :mailer do end end - describe '#has_test_reports?' do - subject { pipeline.has_test_reports? } + describe '#has_reports?' do + subject { pipeline.has_reports?(Ci::JobArtifact.test_reports) } context 'when pipeline has builds with test reports' do before do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 17e2b17a499..ce3b1fe30e6 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -28,7 +28,7 @@ describe Ci::RetryBuildService do job_artifacts_sast job_artifacts_dependency_scanning job_artifacts_container_scanning job_artifacts_dast job_artifacts_license_management job_artifacts_performance - job_artifacts_codequality scheduled_at].freeze + job_artifacts_codequality job_artifacts_metrics scheduled_at].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections