Merge branch 'support-warnings-in-pipeline-creation' into 'master'

Support warnings in pipeline creation

See merge request gitlab-org/gitlab!33762

app/models/ci/pipeline.rb

@@ -51,6 +51,8 @@ module Ci
     has_many :latest_builds, -> { latest }, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'Ci::Build'
     has_many :downloadable_artifacts, -> { not_expired.downloadable }, through: :latest_builds, source: :job_artifacts
 
+    has_many :messages, class_name: 'Ci::PipelineMessage', inverse_of: :pipeline
+
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
     has_many :merge_requests_as_head_pipeline, foreign_key: "head_pipeline_id", class_name: 'MergeRequest'
@@ -634,6 +636,12 @@ module Ci
       yaml_errors.present?
     end
 
+    def add_error_message(content)
+      return unless Gitlab::Ci::Features.store_pipeline_messages?(project)
+
+      messages.error.build(content: content)
+    end
+
     # Manually set the notes for a Ci::Pipeline
     # There is no ActiveRecord relation between Ci::Pipeline and notes
     # as they are related to a commit sha. This method helps importing
@@ -957,7 +965,7 @@ module Ci
       stages.find_by!(name: name)
     end
 
-    def error_messages
+    def full_error_messages
       errors ? errors.full_messages.to_sentence : ""
     end
 

app/models/ci/pipeline_message.rb

+# frozen_string_literal: true
+
+module Ci
+  class PipelineMessage < ApplicationRecord
+    extend Gitlab::Ci::Model
+
+    MAX_CONTENT_LENGTH = 10_000
+
+    belongs_to :pipeline
+
+    validates :content, presence: true
+
+    before_save :truncate_long_content
+
+    enum severity: { error: 0, warning: 1 }
+
+    private
+
+    def truncate_long_content
+      return if content.length <= MAX_CONTENT_LENGTH
+
+      self.content = content.truncate(MAX_CONTENT_LENGTH)
+    end
+  end
+end

app/services/ci/create_pipeline_service.rb

@@ -95,7 +95,7 @@ module Ci
     def execute!(*args, &block)
       execute(*args, &block).tap do |pipeline|
         unless pipeline.persisted?
-          raise CreateError, pipeline.error_messages
+          raise CreateError, pipeline.full_error_messages
         end
       end
     end

changelogs/unreleased/support-warnings-in-pipeline-creation.yml

+---
+title: Store pipeline creation errors and warnings into Ci::PipelineMessage
+merge_request: 33762
+author:
+type: other

db/migrate/20200622104923_create_ci_pipeline_messages_table.rb

+# frozen_string_literal: true
+
+class CreateCiPipelineMessagesTable < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  ERROR_SEVERITY = 0
+  MAX_CONTENT_LENGTH = 10_000
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      create_table :ci_pipeline_messages do |t|
+        t.integer :severity, null: false, default: ERROR_SEVERITY, limit: 2
+        t.references :pipeline, index: true, null: false, foreign_key: { to_table: :ci_pipelines, on_delete: :cascade }, type: :integer
+        t.text :content, null: false
+      end
+    end
+
+    add_text_limit :ci_pipeline_messages, :content, MAX_CONTENT_LENGTH
+  end
+
+  def down
+    drop_table :ci_pipeline_messages
+  end
+end

db/structure.sql

@@ -9936,6 +9936,23 @@ CREATE SEQUENCE public.ci_pipeline_chat_data_id_seq
 
 ALTER SEQUENCE public.ci_pipeline_chat_data_id_seq OWNED BY public.ci_pipeline_chat_data.id;
 
+CREATE TABLE public.ci_pipeline_messages (
+    id bigint NOT NULL,
+    severity smallint DEFAULT 0 NOT NULL,
+    pipeline_id integer NOT NULL,
+    content text NOT NULL,
+    CONSTRAINT check_58ca2981b2 CHECK ((char_length(content) <= 10000))
+);
+
+CREATE SEQUENCE public.ci_pipeline_messages_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.ci_pipeline_messages_id_seq OWNED BY public.ci_pipeline_messages.id;
+
 CREATE TABLE public.ci_pipeline_schedule_variables (
     id integer NOT NULL,
     key character varying NOT NULL,
@@ -16293,6 +16310,8 @@ ALTER TABLE ONLY public.ci_job_variables ALTER COLUMN id SET DEFAULT nextval('pu
 
 ALTER TABLE ONLY public.ci_pipeline_chat_data ALTER COLUMN id SET DEFAULT nextval('public.ci_pipeline_chat_data_id_seq'::regclass);
 
+ALTER TABLE ONLY public.ci_pipeline_messages ALTER COLUMN id SET DEFAULT nextval('public.ci_pipeline_messages_id_seq'::regclass);
+
 ALTER TABLE ONLY public.ci_pipeline_schedule_variables ALTER COLUMN id SET DEFAULT nextval('public.ci_pipeline_schedule_variables_id_seq'::regclass);
 
 ALTER TABLE ONLY public.ci_pipeline_schedules ALTER COLUMN id SET DEFAULT nextval('public.ci_pipeline_schedules_id_seq'::regclass);
@@ -17203,6 +17222,9 @@ ALTER TABLE ONLY public.ci_job_variables
 ALTER TABLE ONLY public.ci_pipeline_chat_data
     ADD CONSTRAINT ci_pipeline_chat_data_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.ci_pipeline_messages
+    ADD CONSTRAINT ci_pipeline_messages_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.ci_pipeline_schedule_variables
     ADD CONSTRAINT ci_pipeline_schedule_variables_pkey PRIMARY KEY (id);
 
@@ -18605,6 +18627,8 @@ CREATE INDEX index_ci_pipeline_chat_data_on_chat_name_id ON public.ci_pipeline_c
 
 CREATE UNIQUE INDEX index_ci_pipeline_chat_data_on_pipeline_id ON public.ci_pipeline_chat_data USING btree (pipeline_id);
 
+CREATE INDEX index_ci_pipeline_messages_on_pipeline_id ON public.ci_pipeline_messages USING btree (pipeline_id);
+
 CREATE UNIQUE INDEX index_ci_pipeline_schedule_variables_on_schedule_id_and_key ON public.ci_pipeline_schedule_variables USING btree (pipeline_schedule_id, key);
 
 CREATE INDEX index_ci_pipeline_schedules_on_next_run_at_and_active ON public.ci_pipeline_schedules USING btree (next_run_at, active);
@@ -21844,6 +21868,9 @@ ALTER TABLE ONLY public.packages_conan_metadata
 ALTER TABLE ONLY public.vulnerability_feedback
     ADD CONSTRAINT fk_rails_8c77e5891a FOREIGN KEY (issue_id) REFERENCES public.issues(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.ci_pipeline_messages
+    ADD CONSTRAINT fk_rails_8d3b04e3e1 FOREIGN KEY (pipeline_id) REFERENCES public.ci_pipelines(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.approval_merge_request_rules_approved_approvers
     ADD CONSTRAINT fk_rails_8dc94cff4d FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE;
 
@@ -23417,6 +23444,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200622070620
 20200622095419
 20200622103836
+20200622104923
 20200622235737
 20200623000148
 20200623000320

ee/app/services/merge_trains/create_pipeline_service.rb

@@ -46,7 +46,7 @@ module MergeTrains
         source_sha: merge_status[:source_id])
         .execute(:merge_request_event, merge_request: merge_request)
 
-      return error(pipeline.error_messages) unless pipeline.persisted?
+      return error(pipeline.full_error_messages) unless pipeline.persisted?
 
       success(pipeline: pipeline)
     end

lib/gitlab/ci/features.rb

@@ -45,6 +45,11 @@ module Gitlab
       def self.release_generation_enabled?
         ::Feature.enabled?(:ci_release_generation)
       end
+
+      # Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/224199
+      def self.store_pipeline_messages?(project)
+        ::Feature.enabled?(:ci_store_pipeline_messages, project, default_enabled: true)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline/chain/helpers.rb

@@ -11,7 +11,12 @@ module Gitlab
               pipeline.yaml_errors = message
             end
 
+            pipeline.add_error_message(message)
             pipeline.drop!(drop_reason) if drop_reason
+
+            # TODO: consider not to rely on AR errors directly as they can be
+            # polluted with other unrelated errors (e.g. state machine)
+            # https://gitlab.com/gitlab-org/gitlab/-/issues/220823
             pipeline.errors.add(:base, message)
           end
         end

spec/lib/gitlab/import_export/all_models.yml

@@ -228,6 +228,7 @@ ci_pipelines:
 - latest_builds
 - daily_report_results
 - latest_builds_report_results
+- messages
 ci_refs:
 - project
 - ci_pipelines

spec/models/ci/pipeline_message_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineMessage do
+  describe 'validations' do
+    subject { described_class.new(pipeline: pipeline, content: content) }
+
+    let(:pipeline) { create(:ci_pipeline) }
+
+    context 'when message content is longer than the limit' do
+      let(:content) { 'x' * (described_class::MAX_CONTENT_LENGTH + 1) }
+
+      it 'is truncated with ellipsis' do
+        subject.save!
+
+        expect(subject.content).to end_with('x...')
+        expect(subject.content.length).to eq(described_class::MAX_CONTENT_LENGTH)
+      end
+    end
+
+    context 'when message is not present' do
+      let(:content) { '' }
+
+      it 'returns an error' do
+        expect(subject.save).to be_falsey
+        expect(subject.errors[:content]).to be_present
+      end
+    end
+
+    context 'when message content is valid' do
+      let(:content) { 'valid message content' }
+
+      it 'is saved with default error severity' do
+        subject.save!
+
+        expect(subject.content).to eq(content)
+        expect(subject.severity).to eq('error')
+        expect(subject).to be_error
+      end
+
+      it 'is persist the defined severity' do
+        subject.severity = :warning
+
+        subject.save!
+
+        expect(subject.content).to eq(content)
+        expect(subject.severity).to eq('warning')
+        expect(subject).to be_warning
+      end
+    end
+  end
+end

spec/models/ci/pipeline_spec.rb

@@ -2634,6 +2634,28 @@ RSpec.describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#add_error_message' do
+    let(:pipeline) { build_stubbed(:ci_pipeline) }
+
+    it 'adds a new pipeline error message' do
+      pipeline.add_error_message('The error message')
+
+      expect(pipeline.messages.map(&:content)).to contain_exactly('The error message')
+    end
+
+    context 'when feature flag ci_store_pipeline_messages is disabled' do
+      before do
+        stub_feature_flags(ci_store_pipeline_messages: false)
+      end
+
+      it ' does not add pipeline error message' do
+        pipeline.add_error_message('The error message')
+
+        expect(pipeline.messages).to be_empty
+      end
+    end
+  end
+
   describe '#has_yaml_errors?' do
     context 'when yaml_errors is set' do
       before do
@@ -3198,8 +3220,8 @@ RSpec.describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe '#error_messages' do
-    subject { pipeline.error_messages }
+  describe '#full_error_messages' do
+    subject { pipeline.full_error_messages }
 
     before do
       pipeline.valid?

spec/services/ci/create_pipeline_service_spec.rb

@@ -194,6 +194,7 @@ RSpec.describe Ci::CreatePipelineService do
 
             expect(head_pipeline).to be_persisted
             expect(head_pipeline.yaml_errors).to be_present
+            expect(head_pipeline.messages).to be_present
             expect(merge_request.reload.head_pipeline).to eq head_pipeline
           end
         end
@@ -1695,6 +1696,7 @@ RSpec.describe Ci::CreatePipelineService do
             expect(pipeline).to be_persisted
             expect(pipeline.builds).to be_empty
             expect(pipeline.yaml_errors).to eq("test_a: needs 'build_a'")
+            expect(pipeline.messages.pluck(:content)).to contain_exactly("test_a: needs 'build_a'")
             expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'")
           end
         end
@@ -1706,6 +1708,7 @@ RSpec.describe Ci::CreatePipelineService do
             expect(pipeline).not_to be_persisted
             expect(pipeline.builds).to be_empty
             expect(pipeline.yaml_errors).to be_nil
+            expect(pipeline.messages).not_to be_empty
             expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'")
           end
         end