Merge branch '7125-refactor-code-quality-similar-to-junit-tests' into 'master'

Resolve "Refactor code quality similar to JUnit tests"

Closes #7125

See merge request gitlab-org/gitlab-ee!7465

doc/ci/examples/code_quality.md

 # Analyze your project's Code Quality
 
 CAUTION: **Caution:**
-The job definition shown below is supported on GitLab 10.4 and later versions.
+The job definition shown below is supported on GitLab 11.4 and later versions.
 For earlier versions, use the [old job definition](#old-job-definition).
 
 CAUTION: **Caution:**
@@ -16,7 +16,7 @@ and Docker.
 
 First, you need GitLab Runner with [docker-in-docker executor][dind].
 
-Once you set up the Runner, add a new job to `.gitlab-ci.yml`, called `code_quality`.
+Once you set up the Runner, add a new job to `.gitlab-ci.yml`.
 
 ```yaml
 code_quality:
@@ -34,22 +34,50 @@ code_quality:
         --volume /var/run/docker.sock:/var/run/docker.sock
         "registry.gitlab.com/gitlab-org/security-products/codequality:$SP_VERSION" /code
   artifacts:
-    paths: [gl-code-quality-report.json]
+    reports:
+      codequality: [gl-code-quality-report.json]
 ```
 
 The above example will create a `code_quality` job in your CI/CD pipeline which
 will scan your source code for code quality issues. The report will be saved
 as an artifact that you can later download and analyze.
 
+TIP: **Tip:**
+Starting with [GitLab Starter][ee] 11.4, this information will be automatically
+extracted and shown right in the merge request widget. To do so, the CI/CD job
+must have a codequality report artifact. Due to implementation limitations we
+always take the latest codequality artifact available.
+[Learn more on Code Quality in merge requests](https://docs.gitlab.com/ee/user/project/merge_requests/code_quality.html).
+
 TIP: **Tip:**
 Starting with [GitLab Starter][ee] 9.3, this information will
 be automatically extracted and shown right in the merge request widget. To do
 so, the CI/CD job must be named `code_quality` and the artifact path must be
 `gl-code-quality-report.json`.
-[Learn more on Code Quality in merge requests](https://docs.gitlab.com/ee/user/project/merge_requests/code_quality.html).
 
 ## Old job definition
 
+For GitLab 11.3 and earlier, the job should look like:
+
+```yaml
+code_quality:
+  image: docker:stable
+  variables:
+    DOCKER_DRIVER: overlay2
+  allow_failure: true
+  services:
+    - docker:stable-dind
+  script:
+    - export SP_VERSION=$(echo "$CI_SERVER_VERSION" | sed 's/^\([0-9]*\)\.\([0-9]*\).*/\1-\2-stable/')
+    - docker run
+        --env SOURCE_CODE="$PWD"
+        --volume "$PWD":/code
+        --volume /var/run/docker.sock:/var/run/docker.sock
+        "registry.gitlab.com/gitlab-org/security-products/codequality:$SP_VERSION" /code
+  artifacts:
+      paths: [gl-code-quality-report.json]
+```
+
 For GitLab 10.3 and earlier, the job should look like:
 
 ```yaml

doc/user/project/merge_requests/code_quality.md

@@ -27,19 +27,19 @@ For instance, consider the following workflow:
 
 ## How it works
 
-In order for the report to show in the merge request, you need to specify a
-`code_quality` job (exact name) that will analyze the code and upload the resulting
-`gl-code-quality-report.json` as an artifact. GitLab will then check this file and show
-the information inside the merge request.
+In order for the report to show in the merge request, you need to specify a job
+that will analyze the code and upload the resulting output JSON as a codequality
+report artifact. GitLab will then check this file and show the information
+inside the merge request.
 
 >**Note:**
 If the Code Climate report doesn't have anything to compare to, no information
 will be displayed in the merge request area. That is the case when you add the
-`code_quality` job in your `.gitlab-ci.yml` for the very first time.
+code quality job in your `.gitlab-ci.yml` for the very first time.
 Consecutive merge requests will have something to compare to and the code quality
 report will be shown properly.
 
-For more information on how the `code_quality` job should look like, check the
+For more information on how the code quality job should look like, check the
 example on [analyzing a project's code quality with Code Climate CLI][cc-docs].
 
 CAUTION: **Caution:**

ee/app/models/ee/ci/build.rb

@@ -7,9 +7,6 @@ module EE
     module Build
       extend ActiveSupport::Concern
 
-      # CODECLIMATE_FILE is deprecated and replaced with CODE_QUALITY_FILE (#5779)
-      CODECLIMATE_FILE = 'codeclimate.json'.freeze
-      CODE_QUALITY_FILE = 'gl-code-quality-report.json'.freeze
       DEPENDENCY_SCANNING_FILE = 'gl-dependency-scanning-report.json'.freeze
       LICENSE_MANAGEMENT_FILE = 'gl-license-management-report.json'.freeze
       SAST_FILE = 'gl-sast-report.json'.freeze
@@ -34,17 +31,6 @@ module EE
         ::Gitlab::Database::LoadBalancing::Sticking.stick(:build, id)
       end
 
-      # has_codeclimate_json? is deprecated and replaced with has_code_quality_json? (#5779)
-      def has_codeclimate_json?
-        name_in?(%w[codeclimate codequality code_quality]) &&
-          has_artifact?(CODECLIMATE_FILE)
-      end
-
-      def has_code_quality_json?
-        name_in?(%w[codeclimate codequality code_quality]) &&
-          has_artifact?(CODE_QUALITY_FILE)
-      end
-
       def has_performance_json?
         name_in?(%w[performance deploy]) &&
           has_artifact?(PERFORMANCE_FILE)
@@ -87,13 +73,13 @@ module EE
         # See https://gitlab.com/gitlab-org/gitlab-ce/issues/46652
       end
 
-      private
-
       def has_artifact?(name)
         options.dig(:artifacts, :paths)&.include?(name) &&
           artifacts_metadata?
       end
 
+      private
+
       def name_in?(names)
         name.in?(Array(names))
       end

ee/app/models/ee/ci/pipeline.rb

@@ -11,18 +11,42 @@ module EE
       prepended do
         has_one :chat_data, class_name: 'Ci::PipelineChatData'
 
+        has_many :job_artifacts, through: :builds
+
         scope :with_security_reports, -> {
           joins(:artifacts).where(ci_builds: { name: %w[sast dependency_scanning sast:container container_scanning dast] })
         }
+
+        # Deprecated, to be removed in 12.0
+        # A hash of Ci::JobArtifact file_types
+        # With mapping to the legacy job names,
+        # that has to contain given files
+        LEGACY_REPORT_FORMATS = {
+          codequality: {
+            names: %w(codeclimate codequality code_quality),
+            files: %w(codeclimate.json gl-code-quality-report.json)
+          }
+        }.freeze
       end
 
-      # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
-      def codeclimate_artifact
-        @codeclimate_artifact ||= artifacts_with_files.find(&:has_codeclimate_json?)
+      def artifact_for_file_type(file_type)
+        job_artifacts.where(file_type: ::Ci::JobArtifact.file_types[file_type]).last
+      end
+
+      def legacy_report_artifact_for_file_type(file_type)
+        legacy_names = LEGACY_REPORT_FORMATS[file_type]
+        return unless legacy_names
+
+        builds.success.latest.where(name: legacy_names[:names]).each do |build|
+          legacy_names[:files].each do |file_name|
+            next unless build.has_artifact?(file_name)
+
+            return OpenStruct.new(build: build, path: file_name)
+          end
         end
 
-      def code_quality_artifact
-        @code_quality_artifact ||= artifacts_with_files.find(&:has_code_quality_json?)
+        # In case there is no artifact return nil
+        nil
       end
 
       def performance_artifact
@@ -83,15 +107,6 @@ module EE
         performance_artifact&.success?
       end
 
-      # has_codeclimate_data? is deprecated and replaced with has_code_quality_data? (#5779)
-      def has_codeclimate_data?
-        codeclimate_artifact&.success?
-      end
-
-      def has_code_quality_data?
-        code_quality_artifact&.success?
-      end
-
       def expose_sast_data?
         project.feature_available?(:sast) &&
           has_sast_data?
@@ -136,15 +151,6 @@ module EE
           expose_container_scanning_data?
       end
 
-      # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data? (#5779)
-      def expose_codeclimate_data?
-        has_codeclimate_data?
-      end
-
-      def expose_code_quality_data?
-        has_code_quality_data?
-      end
-
       private
 
       def artifacts_with_files

ee/app/models/ee/merge_request.rb

@@ -11,11 +11,6 @@ module EE
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :draft_notes
 
-      # codeclimate_artifact is deprecated and replaced with code_quality_artifact (#5779)
-      delegate :codeclimate_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
-      delegate :codeclimate_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
-      delegate :code_quality_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
-      delegate :code_quality_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
       delegate :performance_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
       delegate :performance_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
       delegate :sast_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
@@ -56,17 +51,6 @@ module EE
       false
     end
 
-    # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data? (#5779)
-    def expose_codeclimate_data?
-      !!(head_pipeline&.expose_codeclimate_data? &&
-         base_pipeline&.expose_codeclimate_data?)
-    end
-
-    def expose_code_quality_data?
-      !!(head_pipeline&.expose_code_quality_data? &&
-         base_pipeline&.expose_code_quality_data?)
-    end
-
     def expose_performance_data?
       !!(head_pipeline&.expose_performance_data? &&
          base_pipeline&.expose_performance_data?)

ee/app/presenters/ee/ci/pipeline_presenter.rb

@@ -5,6 +5,24 @@ module EE
         activity_limit_exceeded: 'Pipeline activity limit exceeded!',
         size_limit_exceeded: 'Pipeline size limit exceeded!'
       }.freeze
+
+      def downloadable_url_for_report_type(file_type)
+        if (job_artifact = artifact_for_file_type(file_type)) &&
+            can?(current_user, :read_build, job_artifact.job)
+          return download_project_build_artifacts_url(
+            job_artifact.project,
+            job_artifact.job,
+            file_type: file_type)
+        end
+
+        if (build_artifact = legacy_report_artifact_for_file_type(file_type)) &&
+            can?(current_user, :read_build, build_artifact.build)
+          return raw_project_build_artifacts_url(
+            build_artifact.build.project,
+            build_artifact.build,
+            path: build_artifact.path)
+        end
+      end
     end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -16,33 +16,13 @@ module EE
         end
       end
 
-      # expose_codeclimate_data? is deprecated and replaced with expose_code_quality_data?
-      expose :codeclimate, if: -> (mr, _) { mr.expose_codeclimate_data? } do
-        expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_codeclimate_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.source_project,
-                                          merge_request.head_codeclimate_artifact,
-                                          path: Ci::Build::CODECLIMATE_FILE)
-        end
-
-        expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_codeclimate_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.target_project,
-                                          merge_request.base_codeclimate_artifact,
-                                          path: Ci::Build::CODECLIMATE_FILE)
-        end
-      end
-
-      # We still expose it as `codeclimate` to keep compatibility with Frontend
-      expose :codeclimate, if: -> (mr, _) { mr.expose_code_quality_data? } do
-        expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_code_quality_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.source_project,
-                                          merge_request.head_code_quality_artifact,
-                                          path: Ci::Build::CODE_QUALITY_FILE)
+      expose :codeclimate, if: -> (mr, _) { head_pipeline_downloadable_url_for_report_type(:codequality) } do
+        expose :head_path do |merge_request|
+          head_pipeline_downloadable_url_for_report_type(:codequality)
         end
 
-        expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_code_quality_artifact) } do |merge_request|
-          raw_project_build_artifacts_url(merge_request.target_project,
-                                          merge_request.base_code_quality_artifact,
-                                          path: Ci::Build::CODE_QUALITY_FILE)
+        expose :base_path do |merge_request|
+          base_pipeline_downloadable_url_for_report_type(:codequality)
         end
       end
 
@@ -187,5 +167,15 @@ module EE
         presenter(merge_request).approvals_path
       end
     end
+
+    private
+
+    def head_pipeline_downloadable_url_for_report_type(file_type)
+      object.head_pipeline&.present(current_user: current_user)&.downloadable_url_for_report_type(file_type)
+    end
+
+    def base_pipeline_downloadable_url_for_report_type(file_type)
+      object.base_pipeline&.present(current_user: current_user)&.downloadable_url_for_report_type(file_type)
+    end
   end
 end

ee/changelogs/unreleased/7125-refactor-code-quality-similar-to-junit-tests.yml

+---
+title: Add reports CI syntax for Code Quality reports
+merge_request: 7465
+author:
+type: changed

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -16,6 +16,7 @@ pipelines:
 - triggered_by_pipeline
 - triggered_pipelines
 - chat_data
+- job_artifacts
 protected_branches:
 - unprotect_access_levels
 protected_environments:

ee/spec/models/ci/build_spec.rb

@@ -116,15 +116,6 @@ describe Ci::Build do
   end
 
   build_artifacts_methods = {
-    # has_codeclimate_json? is deprecated and replaced with code_quality_artifact (#5779)
-    has_codeclimate_json?: {
-      filename: Ci::Build::CODECLIMATE_FILE,
-      job_names: %w[codeclimate codequality code_quality]
-    },
-    has_code_quality_json?: {
-      filename: Ci::Build::CODE_QUALITY_FILE,
-      job_names: %w[codeclimate codequality code_quality]
-    },
     has_performance_json?: {
       filename: Ci::Build::PERFORMANCE_FILE,
       job_names: %w[performance deploy]

ee/spec/models/ci/pipeline_spec.rb

@@ -9,6 +9,7 @@ describe Ci::Pipeline do
   end
 
   it { is_expected.to have_one(:chat_data) }
+  it { is_expected.to have_many(:job_artifacts).through(:builds) }
 
   describe '.failure_reasons' do
     it 'contains failure reasons about exceeded limits' do
@@ -18,13 +19,6 @@ describe Ci::Pipeline do
   end
 
   PIPELINE_ARTIFACTS_METHODS = [
-    # codeclimate_artifact is deprecated and replaced with code_quality_artifact  (#5779)
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'codeclimate'] },
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'codequality'] },
-    { method: :codeclimate_artifact, options: [Ci::Build::CODECLIMATE_FILE, 'code_quality'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'codeclimate'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'codequality'] },
-    { method: :code_quality_artifact, options: [Ci::Build::CODE_QUALITY_FILE, 'code_quality'] },
     { method: :performance_artifact, options: [Ci::Build::PERFORMANCE_FILE, 'performance'] },
     { method: :sast_artifact, options: [Ci::Build::SAST_FILE, 'sast'] },
     { method: :dependency_scanning_artifact, options: [Ci::Build::DEPENDENCY_SCANNING_FILE, 'dependency_scanning'] },
@@ -70,7 +64,7 @@ describe Ci::Pipeline do
     end
   end
 
-  %w(sast dependency_scanning dast performance sast_container container_scanning codeclimate code_quality).each do |type|
+  %w(sast dependency_scanning dast performance sast_container container_scanning).each do |type|
     method = "has_#{type}_data?"
 
     describe "##{method}" do
@@ -84,7 +78,7 @@ describe Ci::Pipeline do
     end
   end
 
-  %w(sast dependency_scanning dast performance sast_container container_scanning codeclimate code_quality).each do |type|
+  %w(sast dependency_scanning dast performance sast_container container_scanning).each do |type|
     method = "expose_#{type}_data?"
 
     describe "##{method}" do
@@ -172,6 +166,45 @@ describe Ci::Pipeline do
     end
   end
 
+  describe '#artifact_for_file_type' do
+    let(:file_type) { :codequality }
+    let!(:build) { create(:ci_build, pipeline: pipeline) }
+    let!(:artifact) { create(:ci_job_artifact, :codequality, job: build) }
+
+    subject { pipeline.artifact_for_file_type(file_type) }
+
+    it 'returns the artifact' do
+      expect(subject).to eq(artifact)
+    end
+  end
+
+  describe '#legacy_report_artifact_for_file_type' do
+    let(:file_type) { :codequality }
+    let(:build_name) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:names].first }
+    let(:artifact_path) { ::EE::Ci::Pipeline::LEGACY_REPORT_FORMATS[file_type][:files].first }
+
+    let!(:build) do
+      create(
+        :ci_build,
+        :success,
+        :artifacts,
+        name: build_name,
+        pipeline: pipeline,
+        options: {
+          artifacts: {
+            paths: [artifact_path]
+          }
+        }
+      )
+    end
+
+    subject { pipeline.legacy_report_artifact_for_file_type(:codequality) }
+
+    it 'returns the artifact' do
+      expect(subject).to eq(OpenStruct.new(build: build, path: artifact_path))
+    end
+  end
+
   context 'performance' do
     def create_build(job_name, filename)
       create(
@@ -188,10 +221,10 @@ describe Ci::Pipeline do
     end
 
     it 'does not perform extra queries when calling pipeline artifacts methods after the first' do
-      create_build('codeclimate', 'codeclimate.json')
+      create_build('sast', Ci::Build::SAST_FILE)
       create_build('dependency_scanning', 'gl-dependency-scanning-report.json')
 
-      pipeline.code_quality_artifact
+      pipeline.sast_artifact
 
       expect { pipeline.dependency_scanning_artifact }.not_to exceed_query_limit(0)
     end

ee/spec/models/merge_request_spec.rb

@@ -41,28 +41,6 @@ describe MergeRequest do
     it { expect(subject.base_pipeline).to eq(pipeline) }
   end
 
-  describe '#base_codeclimate_artifact' do
-    before do
-      allow(subject.base_pipeline).to receive(:codeclimate_artifact)
-        .and_return(1)
-    end
-
-    it 'delegates to merge request diff' do
-      expect(subject.base_codeclimate_artifact).to eq(1)
-    end
-  end
-
-  describe '#head_codeclimate_artifact' do
-    before do
-      allow(subject.head_pipeline).to receive(:codeclimate_artifact)
-        .and_return(1)
-    end
-
-    it 'delegates to merge request diff' do
-      expect(subject.head_codeclimate_artifact).to eq(1)
-    end
-  end
-
   describe '#base_performance_artifact' do
     before do
       allow(subject.base_pipeline).to receive(:performance_artifact)
@@ -92,40 +70,6 @@ describe MergeRequest do
     it { is_expected.to delegate_method(:"#{type}_artifact").to(:base_pipeline).with_prefix(:base) }
   end
 
-  describe '#expose_codeclimate_data?' do
-    context 'with codeclimate data' do
-      let(:pipeline) { double(expose_codeclimate_data?: true) }
-
-      before do
-        allow(subject).to receive(:head_pipeline).and_return(pipeline)
-        allow(subject).to receive(:base_pipeline).and_return(pipeline)
-      end
-
-      it { expect(subject.expose_codeclimate_data?).to be_truthy }
-    end
-
-    context 'without codeclimate data' do
-      it { expect(subject.expose_codeclimate_data?).to be_falsey }
-    end
-  end
-
-  describe '#expose_code_quality_data?' do
-    context 'with code_quality data' do
-      let(:pipeline) { double(expose_code_quality_data?: true) }
-
-      before do
-        allow(subject).to receive(:head_pipeline).and_return(pipeline)
-        allow(subject).to receive(:base_pipeline).and_return(pipeline)
-      end
-
-      it { expect(subject.expose_code_quality_data?).to be_truthy }
-    end
-
-    context 'without code_quality data' do
-      it { expect(subject.expose_code_quality_data?).to be_falsey }
-    end
-  end
-
   describe '#expose_performance_data?' do
     context 'with performance data' do
       let(:pipeline) { double(expose_performance_data?: true) }

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -12,7 +12,7 @@ describe MergeRequestWidgetEntity do
   end
 
   subject do
-    described_class.new(merge_request, request: request)
+    described_class.new(merge_request, current_user: user, request: request)
   end
 
   it 'has blob path data' do
@@ -26,32 +26,31 @@ describe MergeRequestWidgetEntity do
     expect(subject.as_json[:blob_path]).to include(:head_path)
   end
 
-  # methods for old artifact are deprecated and replaced with ones for the new name (#5779)
-  it 'has codeclimate data (with old artifact name codeclimate,json)' do
-    build = create(:ci_build, name: 'job')
-
+  describe 'codeclimate' do
+    before do
       allow(merge_request).to receive_messages(
-      expose_codeclimate_data?: true,
-      expose_security_dashboard?: false,
-      base_codeclimate_artifact: build,
-      head_codeclimate_artifact: build
+        base_pipeline: pipeline,
+        head_pipeline: pipeline
       )
-
-    expect(subject.as_json).to include(:codeclimate)
     end
 
-  it 'has codeclimate data (with new artifact name gl-code-quality-report.json)' do
-    build = create(:ci_build, name: 'job')
-
-    allow(merge_request).to receive_messages(
-      expose_code_quality_data?: true,
-      expose_security_dashboard?: false,
-      base_code_quality_artifact: build,
-      head_code_quality_artifact: build
-    )
+    context 'with codeclimate data' do
+      before do
+        job = create(:ci_build, pipeline: pipeline)
+        create(:ci_job_artifact, :codequality, job: job)
+      end
 
+      it 'has codeclimate data entry' do
         expect(subject.as_json).to include(:codeclimate)
       end
+    end
+
+    context 'without codeclimate data' do
+      it 'does not have codeclimate data entry' do
+        expect(subject.as_json).not_to include(:codeclimate)
+      end
+    end
+  end
 
   it 'sets approvals_before_merge to 0 if nil' do
     expect(subject.as_json[:approvals_before_merge]).to eq(0)