Merge branch 'add-ci-lint-class' into 'master'

Extract Gitlab::Ci::Lint class

See merge request gitlab-org/gitlab!39692

app/controllers/projects/ci/lints_controller.rb

@@ -10,40 +10,10 @@ class Projects::Ci::LintsController < Projects::ApplicationController
     @content = params[:content]
     @dry_run = params[:dry_run]
 
-    if @dry_run && Gitlab::Ci::Features.lint_creates_pipeline_with_dry_run?(@project)
-      pipeline = Ci::CreatePipelineService
-        .new(@project, current_user, ref: @project.default_branch)
-        .execute(:push, dry_run: true, content: @content)
-
-      @status = pipeline.error_messages.empty?
-      @stages = pipeline.stages
-      @errors = pipeline.error_messages.map(&:content)
-      @warnings = pipeline.warning_messages.map(&:content)
-    else
-      result = Gitlab::Ci::YamlProcessor.new_with_validation_errors(@content, yaml_processor_options)
-
-      @status = result.valid?
-      @errors = result.errors
-      @warnings = result.warnings
-
-      if result.valid?
-        @config_processor = result.config
-        @stages = @config_processor.stages
-        @builds = @config_processor.builds
-        @jobs = @config_processor.jobs
-      end
-    end
+    @result = Gitlab::Ci::Lint
+      .new(project: @project, current_user: current_user)
+      .validate(@content, dry_run: @dry_run)
 
     render :show
   end
-
-  private
-
-  def yaml_processor_options
-    {
-      project: @project,
-      user: current_user,
-      sha: project.repository.commit.sha
-    }
-  end
 end

app/views/projects/ci/lints/_create.html.haml

-- if @status
+- if @result.valid?
   .bs-callout.bs-callout-success
     %p
       %b= _("Status:")
       = _("syntax is correct")
 
-  = render "projects/ci/lints/lint_warnings", warnings: @warnings
+  = render "projects/ci/lints/lint_warnings", warnings: @result.warnings
 
   .table-holder
     %table.table.table-bordered
@@ -13,54 +13,31 @@
           %th= _("Parameter")
           %th= _("Value")
       %tbody
-        - if @dry_run
-          - @stages.each do |stage|
-            - stage.statuses.each do |job|
-              %tr
-                %td #{stage.name.capitalize} Job - #{job.name}
-                %td
-                  %pre= job.options[:before_script].to_a.join('\n')
-                  %pre= job.options[:script].to_a.join('\n')
-                  %pre= job.options[:after_script].to_a.join('\n')
-                  %br
-                  %b= _("Tag list:")
-                  = job.tag_list.to_a.join(", ") if job.is_a?(Ci::Build)
-                  %br
-                  %b= _("Environment:")
-                  = job.options.dig(:environment, :name)
-                  %br
-                  %b= _("When:")
-                  = job.when
-                  - if job.allow_failure
-                    %b= _("Allowed to fail")
-
-        - else
-          - @stages.each do |stage|
-            - @builds.select { |build| build[:stage] == stage }.each do |build|
-              - job = @jobs[build[:name].to_sym]
-              %tr
-                %td #{stage.capitalize} Job - #{build[:name]}
-                %td
-                  %pre= job[:before_script].to_a.join('\n')
-                  %pre= job[:script].to_a.join('\n')
-                  %pre= job[:after_script].to_a.join('\n')
-                  %br
-                  %b= _("Tag list:")
-                  = build[:tag_list].to_a.join(", ")
-                  %br
-                  %b= _("Only policy:")
-                  = job[:only].to_a.join(", ")
-                  %br
-                  %b= _("Except policy:")
-                  = job[:except].to_a.join(", ")
-                  %br
-                  %b= _("Environment:")
-                  = build[:environment]
-                  %br
-                  %b= _("When:")
-                  = build[:when]
-                  - if build[:allow_failure]
-                    %b= _("Allowed to fail")
+        - @result.jobs.each do |job|
+          %tr
+            %td #{job[:stage].capitalize} Job - #{job[:name]}
+            %td
+              %pre= job[:before_script].to_a.join('\n')
+              %pre= job[:script].to_a.join('\n')
+              %pre= job[:after_script].to_a.join('\n')
+              %br
+              %b= _("Tag list:")
+              = job[:tag_list].to_a.join(", ")
+              - unless @dry_run
+                %br
+                %b= _("Only policy:")
+                = job[:only].to_a.join(", ")
+                %br
+                %b= _("Except policy:")
+                = job[:except].to_a.join(", ")
+              %br
+              %b= _("Environment:")
+              = job[:environment]
+              %br
+              %b= _("When:")
+              = job[:when]
+              - if job[:allow_failure]
+                %b= _("Allowed to fail")
 
 - else
   .bs-callout.bs-callout-danger
@@ -68,7 +45,7 @@
       %b= _("Status:")
       = _("syntax is incorrect")
   %pre
-    - @errors.each do |message|
+    - @result.errors.each do |message|
       %p= message
 
-  = render "projects/ci/lints/lint_warnings", warnings: @warnings
+  = render "projects/ci/lints/lint_warnings", warnings: @result.warnings

app/views/projects/ci/lints/show.html.haml

@@ -27,4 +27,4 @@
   .row.prepend-top-20
     .col-sm-12
       .results.project-ci-template
-        = render partial: 'create' if defined?(@status)
+        = render partial: 'create' if defined?(@result)

lib/gitlab/ci/lint.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class Lint
+      class Result
+        attr_reader :jobs, :errors, :warnings
+
+        def initialize(jobs:, errors:, warnings:)
+          @jobs = jobs
+          @errors = errors
+          @warnings = warnings
+        end
+
+        def valid?
+          @errors.empty?
+        end
+      end
+
+      def initialize(project:, current_user:)
+        @project = project
+        @current_user = current_user
+      end
+
+      def validate(content, dry_run: false)
+        if dry_run && Gitlab::Ci::Features.lint_creates_pipeline_with_dry_run?(@project)
+          simulate_pipeline_creation(content)
+        else
+          static_validation(content)
+        end
+      end
+
+      private
+
+      def simulate_pipeline_creation(content)
+        pipeline = ::Ci::CreatePipelineService
+          .new(@project, @current_user, ref: @project.default_branch)
+          .execute(:push, dry_run: true, content: content)
+
+        Result.new(
+          jobs: dry_run_convert_to_jobs(pipeline.stages),
+          errors: pipeline.error_messages.map(&:content),
+          warnings: pipeline.warning_messages.map(&:content)
+        )
+      end
+
+      def static_validation(content)
+        result = Gitlab::Ci::YamlProcessor.new_with_validation_errors(
+          content,
+          project: @project,
+          user: @current_user,
+          sha: @project.repository.commit.sha)
+
+        Result.new(
+          jobs: static_validation_convert_to_jobs(result.config&.stages, result.config&.builds),
+          errors: result.errors,
+          warnings: result.warnings
+        )
+      end
+
+      def dry_run_convert_to_jobs(stages)
+        stages.reduce([]) do |jobs, stage|
+          jobs + stage.statuses.map do |job|
+            {
+              name: job.name,
+              stage: stage.name,
+              before_script: job.options[:before_script],
+              script: job.options[:script],
+              after_script: job.options[:after_script],
+              tag_list: (job.tag_list if job.is_a?(::Ci::Build)),
+              environment: job.options.dig(:environment, :name),
+              when: job.when,
+              allow_failure: job.allow_failure
+            }
+          end
+        end
+      end
+
+      def static_validation_convert_to_jobs(stages, all_jobs)
+        jobs = []
+        return jobs unless stages || all_jobs
+
+        stages.each do |stage_name|
+          all_jobs.each do |job|
+            next unless job[:stage] == stage_name
+
+            jobs << {
+              name: job[:name],
+              stage: stage_name,
+              before_script: job.dig(:options, :before_script),
+              script: job.dig(:options, :script),
+              after_script: job.dig(:options, :after_script),
+              tag_list: job[:tag_list].to_a,
+              only: job[:only],
+              except: job[:except],
+              environment: job[:environment],
+              when: job[:when],
+              allow_failure: job[:allow_failure]
+            }
+          end
+        end
+
+        jobs
+      end
+    end
+  end
+end

spec/controllers/projects/ci/lints_controller_spec.rb

@@ -147,19 +147,19 @@ RSpec.describe Projects::Ci::LintsController do
         project.add_developer(user)
       end
 
-      it 'assigns errors' do
+      it 'assigns result with errors' do
         subject
 
-        expect(assigns[:errors]).to eq(['root config contains unknown keys: rubocop'])
+        expect(assigns[:result].errors).to eq(['root config contains unknown keys: rubocop'])
       end
 
       context 'with dry_run mode' do
         subject { post :create, params: params.merge(dry_run: 'true') }
 
-        it 'assigns errors' do
+        it 'assigns result with errors' do
           subject
 
-          expect(assigns[:errors]).to eq(['root config contains unknown keys: rubocop'])
+          expect(assigns[:result].errors).to eq(['root config contains unknown keys: rubocop'])
         end
       end
     end

spec/lib/gitlab/ci/lint_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Lint do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+
+  let(:lint) { described_class.new(project: project, current_user: user) }
+
+  describe '#validate' do
+    subject { lint.validate(content, dry_run: dry_run) }
+
+    shared_examples 'content is valid' do
+      let(:content) do
+        <<~YAML
+        build:
+          stage: build
+          before_script:
+            - before_build
+          script: echo
+          environment: staging
+          when: manual
+        rspec:
+          stage: test
+          script: rspec
+          after_script:
+            - after_rspec
+          tags: [docker]
+        YAML
+      end
+
+      it 'returns a valid result', :aggregate_failures do
+        expect(subject).to be_valid
+
+        expect(subject.errors).to be_empty
+        expect(subject.warnings).to be_empty
+        expect(subject.jobs).to be_present
+
+        build_job = subject.jobs.first
+        expect(build_job[:name]).to eq('build')
+        expect(build_job[:stage]).to eq('build')
+        expect(build_job[:before_script]).to eq(['before_build'])
+        expect(build_job[:script]).to eq(['echo'])
+        expect(build_job.fetch(:after_script)).to be_nil
+        expect(build_job[:tag_list]).to eq([])
+        expect(build_job[:environment]).to eq('staging')
+        expect(build_job[:when]).to eq('manual')
+        expect(build_job[:allow_failure]).to eq(true)
+
+        rspec_job = subject.jobs.last
+        expect(rspec_job[:name]).to eq('rspec')
+        expect(rspec_job[:stage]).to eq('test')
+        expect(rspec_job.fetch(:before_script)).to be_nil
+        expect(rspec_job[:script]).to eq(['rspec'])
+        expect(rspec_job[:after_script]).to eq(['after_rspec'])
+        expect(rspec_job[:tag_list]).to eq(['docker'])
+        expect(rspec_job.fetch(:environment)).to be_nil
+        expect(rspec_job[:when]).to eq('on_success')
+        expect(rspec_job[:allow_failure]).to eq(false)
+      end
+    end
+
+    shared_examples 'content with errors and warnings' do
+      context 'when content has errors' do
+        let(:content) do
+          <<~YAML
+          build:
+            invalid: syntax
+          YAML
+        end
+
+        it 'returns a result with errors' do
+          expect(subject).not_to be_valid
+          expect(subject.errors).to include(/root config contains unknown keys/)
+        end
+      end
+
+      context 'when content has warnings' do
+        let(:content) do
+          <<~YAML
+          rspec:
+            script: rspec
+            rules:
+              - when: always
+          YAML
+        end
+
+        it 'returns a result with warnings' do
+          expect(subject).to be_valid
+          expect(subject.warnings).to include(/rspec may allow multiple pipelines to run/)
+        end
+      end
+
+      context 'when content has errors and warnings' do
+        let(:content) do
+          <<~YAML
+          rspec:
+            script: rspec
+            rules:
+              - when: always
+          karma:
+            script: karma
+            unknown: key
+          YAML
+        end
+
+        it 'returns a result with errors and warnings' do
+          expect(subject).not_to be_valid
+          expect(subject.errors).to include(/karma config contains unknown keys/)
+          expect(subject.warnings).to include(/rspec may allow multiple pipelines to run/)
+        end
+      end
+    end
+
+    shared_context 'advanced validations' do
+      let(:content) do
+        <<~YAML
+        build:
+          stage: build
+          script: echo
+          rules:
+            - if: '$CI_MERGE_REQUEST_ID'
+        test:
+          stage: test
+          script: echo
+          needs: [build]
+        YAML
+      end
+    end
+
+    context 'when user has permissions to write the ref' do
+      before do
+        project.add_developer(user)
+      end
+
+      context 'when using default static mode' do
+        let(:dry_run) { false }
+
+        it_behaves_like 'content with errors and warnings'
+
+        it_behaves_like 'content is valid' do
+          it 'includes extra attributes' do
+            subject.jobs.each do |job|
+              expect(job[:only]).to eq(refs: %w[branches tags])
+              expect(job.fetch(:except)).to be_nil
+            end
+          end
+        end
+
+        include_context 'advanced validations' do
+          it 'does not catch advanced logical errors' do
+            expect(subject).to be_valid
+            expect(subject.errors).to be_empty
+          end
+        end
+
+        it 'uses YamlProcessor' do
+          expect(Gitlab::Ci::YamlProcessor)
+            .to receive(:new_with_validation_errors)
+            .and_call_original
+
+          subject
+        end
+      end
+
+      context 'when using dry run mode' do
+        let(:dry_run) { true }
+
+        it_behaves_like 'content with errors and warnings'
+
+        it_behaves_like 'content is valid' do
+          it 'does not include extra attributes' do
+            subject.jobs.each do |job|
+              expect(job.key?(:only)).to be_falsey
+              expect(job.key?(:except)).to be_falsey
+            end
+          end
+        end
+
+        include_context 'advanced validations' do
+          it 'runs advanced logical validations' do
+            expect(subject).not_to be_valid
+            expect(subject.errors).to eq(["test: needs 'build'"])
+          end
+        end
+
+        it 'uses Ci::CreatePipelineService' do
+          expect(::Ci::CreatePipelineService)
+            .to receive(:new)
+            .and_call_original
+
+          subject
+        end
+      end
+    end
+
+    context 'when user does not have permissions to write the ref' do
+      before do
+        project.add_reporter(user)
+      end
+
+      context 'when using default static mode' do
+        let(:dry_run) { false }
+
+        it_behaves_like 'content is valid'
+      end
+
+      context 'when using dry run mode' do
+        let(:dry_run) { true }
+
+        let(:content) do
+          <<~YAML
+          job:
+            script: echo
+          YAML
+        end
+
+        it 'does not allow validation' do
+          expect(subject).not_to be_valid
+          expect(subject.errors).to include('Insufficient permissions to create a new pipeline')
+        end
+      end
+    end
+  end
+end

spec/views/projects/ci/lints/show.html.haml_spec.rb

@@ -4,16 +4,15 @@ require 'spec_helper'
 
 RSpec.describe 'projects/ci/lints/show' do
   include Devise::Test::ControllerHelpers
-  let(:project) { create(:project, :repository) }
-  let(:config_processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(content)) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository) }
+  let(:lint) { Gitlab::Ci::Lint.new(project: project, current_user: user) }
+  let(:result) { lint.validate(YAML.dump(content)) }
 
   describe 'XSS protection' do
     before do
       assign(:project, project)
-      assign(:status, true)
-      assign(:builds, config_processor.builds)
-      assign(:stages, config_processor.stages)
-      assign(:jobs, config_processor.jobs)
+      assign(:result, result)
     end
 
     context 'when builds attrbiutes contain HTML nodes' do
@@ -66,10 +65,7 @@ RSpec.describe 'projects/ci/lints/show' do
 
     before do
       assign(:project, project)
-      assign(:status, true)
-      assign(:builds, config_processor.builds)
-      assign(:stages, config_processor.stages)
-      assign(:jobs, config_processor.jobs)
+      assign(:result, result)
     end
 
     it 'shows the correct values' do
@@ -85,7 +81,7 @@ RSpec.describe 'projects/ci/lints/show' do
 
     context 'when content has warnings' do
       before do
-        assign(:warnings, ['Warning 1', 'Warning 2'])
+        allow(result).to receive(:warnings).and_return(['Warning 1', 'Warning 2'])
       end
 
       it 'shows warning messages' do
@@ -99,11 +95,14 @@ RSpec.describe 'projects/ci/lints/show' do
   end
 
   context 'when the content is invalid' do
+    let(:content) { double(:content) }
+
     before do
+      allow(result).to receive(:warnings).and_return(['Warning 1', 'Warning 2'])
+      allow(result).to receive(:errors).and_return(['Undefined error'])
+
       assign(:project, project)
-      assign(:status, false)
-      assign(:errors, ['Undefined error'])
-      assign(:warnings, ['Warning 1', 'Warning 2'])
+      assign(:result, result)
     end
 
     it 'shows error message' do