From ca6a1f33f91a8cceadebfb9c4e9ac6afa340f71d Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Fri, 9 Aug 2019 11:40:45 +0200 Subject: CE port for pipelines for external pull requests Detect if pipeline runs for a GitHub pull request When using a mirror for CI/CD only we register a pull_request webhook. When a pull_request webhook is received, if the source branch SHA matches the actual head of the branch in the repository we create immediately a new pipeline for the external pull request. Otherwise we store the pull request info for when the push webhook is received. When using "only/except: external_pull_requests" we can detect if the pipeline has a open pull request on GitHub and create or not the job based on that. --- app/models/ci/pipeline.rb | 10 + app/models/ci/pipeline_enums.rb | 3 +- app/models/external_pull_request.rb | 96 +++++++++ app/models/project.rb | 2 + app/services/ci/create_pipeline_service.rb | 5 +- .../create_pipeline_service.rb | 29 +++ app/workers/all_queues.yml | 1 + .../update_external_pull_requests_worker.rb | 25 +++ config/sidekiq_queues.yml | 1 + ...20190829131130_create_external_pull_requests.rb | 25 +++ ...add_external_pull_request_id_to_ci_pipelines.rb | 15 ++ ..._index_to_ci_pipelines_external_pull_request.rb | 17 ++ ...gn_key_to_ci_pipelines_external_pull_request.rb | 17 ++ db/schema.rb | 19 ++ lib/gitlab/ci/pipeline/chain/build.rb | 1 + lib/gitlab/ci/pipeline/chain/command.rb | 2 +- lib/gitlab/import_export/import_export.yml | 2 + spec/factories/external_pull_requests.rb | 17 ++ spec/lib/gitlab/ci/build/policy/refs_spec.rb | 14 ++ spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 34 ++++ spec/lib/gitlab/import_export/all_models.yml | 4 + .../gitlab/import_export/safe_model_attributes.yml | 14 ++ spec/models/ci/pipeline_spec.rb | 20 ++ spec/models/external_pull_request_spec.rb | 220 +++++++++++++++++++++ spec/models/project_spec.rb | 1 + spec/services/ci/create_pipeline_service_spec.rb | 154 ++++++++++++++- .../create_pipeline_service_spec.rb | 72 +++++++ .../update_external_pull_requests_worker_spec.rb | 54 +++++ 28 files changed, 869 insertions(+), 5 deletions(-) create mode 100644 app/models/external_pull_request.rb create mode 100644 app/services/external_pull_requests/create_pipeline_service.rb create mode 100644 app/workers/update_external_pull_requests_worker.rb create mode 100644 db/migrate/20190829131130_create_external_pull_requests.rb create mode 100644 db/migrate/20190830075508_add_external_pull_request_id_to_ci_pipelines.rb create mode 100644 db/migrate/20190830080123_add_index_to_ci_pipelines_external_pull_request.rb create mode 100644 db/migrate/20190830080626_add_foreign_key_to_ci_pipelines_external_pull_request.rb create mode 100644 spec/factories/external_pull_requests.rb create mode 100644 spec/models/external_pull_request_spec.rb create mode 100644 spec/services/external_pull_requests/create_pipeline_service_spec.rb create mode 100644 spec/workers/update_external_pull_requests_worker_spec.rb diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 2b6f10ef79f..e34c6204742 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -23,6 +23,7 @@ module Ci belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline' belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule' belongs_to :merge_request, class_name: 'MergeRequest' + belongs_to :external_pull_request has_internal_id :iid, scope: :project, presence: false, init: ->(s) do s&.project&.all_pipelines&.maximum(:iid) || s&.project&.all_pipelines&.count @@ -64,6 +65,11 @@ module Ci validates :merge_request, presence: { if: :merge_request_event? } validates :merge_request, absence: { unless: :merge_request_event? } validates :tag, inclusion: { in: [false], if: :merge_request_event? } + + validates :external_pull_request, presence: { if: :external_pull_request_event? } + validates :external_pull_request, absence: { unless: :external_pull_request_event? } + validates :tag, inclusion: { in: [false], if: :external_pull_request_event? } + validates :status, presence: { unless: :importing? } validate :valid_commit_sha, unless: :importing? validates :source, exclusion: { in: %w(unknown), unless: :importing? }, on: :create @@ -675,6 +681,10 @@ module Ci variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA', value: target_sha.to_s) variables.concat(merge_request.predefined_variables) end + + if external_pull_request_event? && external_pull_request + variables.concat(external_pull_request.predefined_variables) + end end end diff --git a/app/models/ci/pipeline_enums.rb b/app/models/ci/pipeline_enums.rb index 571c4271475..0c2bd0aa8eb 100644 --- a/app/models/ci/pipeline_enums.rb +++ b/app/models/ci/pipeline_enums.rb @@ -23,7 +23,8 @@ module Ci api: 5, external: 6, chat: 8, - merge_request_event: 10 + merge_request_event: 10, + external_pull_request_event: 11 } end diff --git a/app/models/external_pull_request.rb b/app/models/external_pull_request.rb new file mode 100644 index 00000000000..65ae8d95500 --- /dev/null +++ b/app/models/external_pull_request.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +# This model stores pull requests coming from external providers, such as +# GitHub, when GitLab project is set as CI/CD only and remote mirror. +# +# When setting up a remote mirror with GitHub we subscribe to push and +# pull_request webhook events. When a pull request is opened on GitHub, +# a webhook is sent out, we create or update the status of the pull +# request locally. +# +# When the mirror is updated and changes are pushed to branches we check +# if there are open pull requests for the source and target branch. +# If so, we create pipelines for external pull requests. +class ExternalPullRequest < ApplicationRecord + include Gitlab::Utils::StrongMemoize + include ShaAttribute + + belongs_to :project + + sha_attribute :source_sha + sha_attribute :target_sha + + validates :source_branch, presence: true + validates :target_branch, presence: true + validates :source_sha, presence: true + validates :target_sha, presence: true + validates :source_repository, presence: true + validates :target_repository, presence: true + validates :status, presence: true + + enum status: { + open: 1, + closed: 2 + } + + # We currently don't support pull requests from fork, so + # we are going to return an error to the webhook + validate :not_from_fork + + scope :by_source_branch, ->(branch) { where(source_branch: branch) } + scope :by_source_repository, -> (repository) { where(source_repository: repository) } + + def self.create_or_update_from_params(params) + find_params = params.slice(:project_id, :source_branch, :target_branch) + + safe_find_or_initialize_and_update(find: find_params, update: params) do |pull_request| + yield(pull_request) if block_given? + end + end + + def actual_branch_head? + actual_source_branch_sha == source_sha + end + + def from_fork? + source_repository != target_repository + end + + def source_ref + Gitlab::Git::BRANCH_REF_PREFIX + source_branch + end + + def predefined_variables + Gitlab::Ci::Variables::Collection.new.tap do |variables| + variables.append(key: 'CI_EXTERNAL_PULL_REQUEST_IID', value: pull_request_iid.to_s) + variables.append(key: 'CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_SHA', value: source_sha) + variables.append(key: 'CI_EXTERNAL_PULL_REQUEST_TARGET_BRANCH_SHA', value: target_sha) + variables.append(key: 'CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_NAME', value: source_branch) + variables.append(key: 'CI_EXTERNAL_PULL_REQUEST_TARGET_BRANCH_NAME', value: target_branch) + end + end + + private + + def actual_source_branch_sha + project.commit(source_ref)&.sha + end + + def not_from_fork + if from_fork? + errors.add(:base, 'Pull requests from fork are not supported') + end + end + + def self.safe_find_or_initialize_and_update(find:, update:) + safe_ensure_unique(retries: 1) do + model = find_or_initialize_by(find) + + if model.update(update) + yield(model) if block_given? + end + + model + end + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 17b52d0578e..d948410e397 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -291,6 +291,8 @@ class Project < ApplicationRecord has_many :remote_mirrors, inverse_of: :project has_many :cycle_analytics_stages, class_name: 'Analytics::CycleAnalytics::ProjectStage' + has_many :external_pull_requests, inverse_of: :project + accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :import_data diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 29317f1176e..94b581a0eef 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -18,7 +18,8 @@ module Ci Gitlab::Ci::Pipeline::Chain::Limit::Activity, Gitlab::Ci::Pipeline::Chain::Limit::JobActivity].freeze - def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, merge_request: nil, **options, &block) + # rubocop: disable Metrics/ParameterLists + def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, merge_request: nil, external_pull_request: nil, **options, &block) @pipeline = Ci::Pipeline.new command = Gitlab::Ci::Pipeline::Chain::Command.new( @@ -32,6 +33,7 @@ module Ci trigger_request: trigger_request, schedule: schedule, merge_request: merge_request, + external_pull_request: external_pull_request, ignore_skip_ci: ignore_skip_ci, save_incompleted: save_on_errors, seeds_block: block, @@ -62,6 +64,7 @@ module Ci pipeline end + # rubocop: enable Metrics/ParameterLists def execute!(*args, &block) execute(*args, &block).tap do |pipeline| diff --git a/app/services/external_pull_requests/create_pipeline_service.rb b/app/services/external_pull_requests/create_pipeline_service.rb new file mode 100644 index 00000000000..36411465ff1 --- /dev/null +++ b/app/services/external_pull_requests/create_pipeline_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +# This service is responsible for creating a pipeline for a given +# ExternalPullRequest coming from other providers such as GitHub. + +module ExternalPullRequests + class CreatePipelineService < BaseService + def execute(pull_request) + return unless pull_request.open? && pull_request.actual_branch_head? + + create_pipeline_for(pull_request) + end + + private + + def create_pipeline_for(pull_request) + Ci::CreatePipelineService.new(project, current_user, create_params(pull_request)) + .execute(:external_pull_request_event, external_pull_request: pull_request) + end + + def create_params(pull_request) + { + ref: pull_request.source_ref, + source_sha: pull_request.source_sha, + target_sha: pull_request.target_sha + } + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 991a177018e..a33afd436b0 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -160,6 +160,7 @@ - repository_import - repository_remove_remote - system_hook_push +- update_external_pull_requests - update_merge_requests - update_project_statistics - upload_checksum diff --git a/app/workers/update_external_pull_requests_worker.rb b/app/workers/update_external_pull_requests_worker.rb new file mode 100644 index 00000000000..c5acfa82ada --- /dev/null +++ b/app/workers/update_external_pull_requests_worker.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class UpdateExternalPullRequestsWorker + include ApplicationWorker + + def perform(project_id, user_id, ref) + project = Project.find_by_id(project_id) + return unless project + + user = User.find_by_id(user_id) + return unless user + + branch = Gitlab::Git.branch_name(ref) + return unless branch + + external_pull_requests = project.external_pull_requests + .by_source_repository(project.import_source) + .by_source_branch(branch) + + external_pull_requests.find_each do |pull_request| + ExternalPullRequests::CreatePipelineService.new(project, user) + .execute(pull_request) + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 7edec576f9a..e89e9657314 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -115,3 +115,4 @@ - [export_csv, 1] - [incident_management, 2] - [jira_connect, 1] + - [update_external_pull_requests, 3] diff --git a/db/migrate/20190829131130_create_external_pull_requests.rb b/db/migrate/20190829131130_create_external_pull_requests.rb new file mode 100644 index 00000000000..0c3168807ec --- /dev/null +++ b/db/migrate/20190829131130_create_external_pull_requests.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class CreateExternalPullRequests < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX = 'index_external_pull_requests_on_project_and_branches' + + def change + create_table :external_pull_requests do |t| + t.timestamps_with_timezone null: false + t.references :project, null: false, foreign_key: { on_delete: :cascade }, index: false + t.integer :pull_request_iid, null: false + t.integer :status, null: false, limit: 2 + t.string :source_branch, null: false, limit: 255 + t.string :target_branch, null: false, limit: 255 + t.string :source_repository, null: false, limit: 255 + t.string :target_repository, null: false, limit: 255 + t.binary :source_sha, null: false + t.binary :target_sha, null: false + + t.index [:project_id, :source_branch, :target_branch], unique: true, name: INDEX + end + end +end diff --git a/db/migrate/20190830075508_add_external_pull_request_id_to_ci_pipelines.rb b/db/migrate/20190830075508_add_external_pull_request_id_to_ci_pipelines.rb new file mode 100644 index 00000000000..5abf56742b1 --- /dev/null +++ b/db/migrate/20190830075508_add_external_pull_request_id_to_ci_pipelines.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddExternalPullRequestIdToCiPipelines < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column :ci_pipelines, :external_pull_request_id, :bigint + end + + def down + remove_column :ci_pipelines, :external_pull_request_id + end +end diff --git a/db/migrate/20190830080123_add_index_to_ci_pipelines_external_pull_request.rb b/db/migrate/20190830080123_add_index_to_ci_pipelines_external_pull_request.rb new file mode 100644 index 00000000000..d2f5ad7a420 --- /dev/null +++ b/db/migrate/20190830080123_add_index_to_ci_pipelines_external_pull_request.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToCiPipelinesExternalPullRequest < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_pipelines, :external_pull_request_id, where: 'external_pull_request_id IS NOT NULL' + end + + def down + remove_concurrent_index :ci_pipelines, :external_pull_request_id + end +end diff --git a/db/migrate/20190830080626_add_foreign_key_to_ci_pipelines_external_pull_request.rb b/db/migrate/20190830080626_add_foreign_key_to_ci_pipelines_external_pull_request.rb new file mode 100644 index 00000000000..b38fda83047 --- /dev/null +++ b/db/migrate/20190830080626_add_foreign_key_to_ci_pipelines_external_pull_request.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddForeignKeyToCiPipelinesExternalPullRequest < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :ci_pipelines, :external_pull_requests, column: :external_pull_request_id, on_delete: :nullify + end + + def down + remove_foreign_key :ci_pipelines, :external_pull_requests + end +end diff --git a/db/schema.rb b/db/schema.rb index 98c4403efe1..9301aa5a8de 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -752,7 +752,9 @@ ActiveRecord::Schema.define(version: 2019_09_04_173203) do t.integer "merge_request_id" t.binary "source_sha" t.binary "target_sha" + t.bigint "external_pull_request_id" t.index ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id" + t.index ["external_pull_request_id"], name: "index_ci_pipelines_on_external_pull_request_id", where: "(external_pull_request_id IS NOT NULL)" t.index ["merge_request_id"], name: "index_ci_pipelines_on_merge_request_id", where: "(merge_request_id IS NOT NULL)" t.index ["pipeline_schedule_id"], name: "index_ci_pipelines_on_pipeline_schedule_id" t.index ["project_id", "iid"], name: "index_ci_pipelines_on_project_id_and_iid", unique: true, where: "(iid IS NOT NULL)" @@ -1321,6 +1323,21 @@ ActiveRecord::Schema.define(version: 2019_09_04_173203) do t.index ["target_type", "target_id"], name: "index_events_on_target_type_and_target_id" end + create_table "external_pull_requests", force: :cascade do |t| + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + t.bigint "project_id", null: false + t.integer "pull_request_iid", null: false + t.integer "status", limit: 2, null: false + t.string "source_branch", limit: 255, null: false + t.string "target_branch", limit: 255, null: false + t.string "source_repository", limit: 255, null: false + t.string "target_repository", limit: 255, null: false + t.binary "source_sha", null: false + t.binary "target_sha", null: false + t.index ["project_id", "source_branch", "target_branch"], name: "index_external_pull_requests_on_project_and_branches", unique: true + end + create_table "feature_gates", id: :serial, force: :cascade do |t| t.string "feature_key", null: false t.string "key", null: false @@ -3781,6 +3798,7 @@ ActiveRecord::Schema.define(version: 2019_09_04_173203) do add_foreign_key "ci_pipeline_variables", "ci_pipelines", column: "pipeline_id", name: "fk_f29c5f4380", on_delete: :cascade add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify + add_foreign_key "ci_pipelines", "external_pull_requests", name: "fk_190998ef09", on_delete: :nullify add_foreign_key "ci_pipelines", "merge_requests", name: "fk_a23be95014", on_delete: :cascade add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade add_foreign_key "ci_runner_namespaces", "ci_runners", column: "runner_id", on_delete: :cascade @@ -3845,6 +3863,7 @@ ActiveRecord::Schema.define(version: 2019_09_04_173203) do add_foreign_key "events", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "events", "projects", on_delete: :cascade add_foreign_key "events", "users", column: "author_id", name: "fk_edfd187b6f", on_delete: :cascade + add_foreign_key "external_pull_requests", "projects", on_delete: :cascade add_foreign_key "fork_network_members", "fork_networks", on_delete: :cascade add_foreign_key "fork_network_members", "projects", column: "forked_from_project_id", name: "fk_b01280dae4", on_delete: :nullify add_foreign_key "fork_network_members", "projects", on_delete: :cascade diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index 164a4634d84..899df81ea5c 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -19,6 +19,7 @@ module Gitlab user: @command.current_user, pipeline_schedule: @command.schedule, merge_request: @command.merge_request, + external_pull_request: @command.external_pull_request, variables_attributes: Array(@command.variables_attributes) ) diff --git a/lib/gitlab/ci/pipeline/chain/command.rb b/lib/gitlab/ci/pipeline/chain/command.rb index afad391e8e0..58f89a6be5e 100644 --- a/lib/gitlab/ci/pipeline/chain/command.rb +++ b/lib/gitlab/ci/pipeline/chain/command.rb @@ -7,7 +7,7 @@ module Gitlab Command = Struct.new( :source, :project, :current_user, :origin_ref, :checkout_sha, :after_sha, :before_sha, :source_sha, :target_sha, - :trigger_request, :schedule, :merge_request, + :trigger_request, :schedule, :merge_request, :external_pull_request, :ignore_skip_ci, :save_incompleted, :seeds_block, :variables_attributes, :push_options, :chat_data, :allow_mirror_update diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index bd0f3e70749..40acb24d191 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -64,6 +64,8 @@ project_tree: - :push_event_payload - stages: - :statuses + - :external_pull_request + - :external_pull_requests - :auto_devops - :triggers - :pipeline_schedules diff --git a/spec/factories/external_pull_requests.rb b/spec/factories/external_pull_requests.rb new file mode 100644 index 00000000000..08d0fa4d419 --- /dev/null +++ b/spec/factories/external_pull_requests.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :external_pull_request do + sequence(:pull_request_iid) + project + source_branch 'feature' + source_repository 'the-repository' + source_sha '97de212e80737a608d939f648d959671fb0a0142' + target_branch 'master' + target_repository 'the-repository' + target_sha 'a09386439ca39abe575675ffd4b89ae824fec22f' + status :open + + trait(:closed) { status 'closed' } + end +end diff --git a/spec/lib/gitlab/ci/build/policy/refs_spec.rb b/spec/lib/gitlab/ci/build/policy/refs_spec.rb index 43c5d3ec980..8fc1e0a4e88 100644 --- a/spec/lib/gitlab/ci/build/policy/refs_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/refs_spec.rb @@ -84,6 +84,20 @@ describe Gitlab::Ci::Build::Policy::Refs do .not_to be_satisfied_by(pipeline) end end + + context 'when source is external_pull_request_event' do + let(:pipeline) { build_stubbed(:ci_pipeline, source: :external_pull_request_event) } + + it 'is satisfied with only: external_pull_request' do + expect(described_class.new(%w[external_pull_requests])) + .to be_satisfied_by(pipeline) + end + + it 'is not satisfied with only: external_pull_request_event' do + expect(described_class.new(%w[external_pull_request_events])) + .not_to be_satisfied_by(pipeline) + end + end end context 'when matching a ref by a regular expression' do diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index bf9ff922c05..ba4f841cf43 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -128,4 +128,38 @@ describe Gitlab::Ci::Pipeline::Chain::Build do expect(pipeline.target_sha).to eq(merge_request.target_branch_sha) end end + + context 'when pipeline is running for an external pull request' do + let(:command) do + Gitlab::Ci::Pipeline::Chain::Command.new( + source: :external_pull_request_event, + origin_ref: 'feature', + checkout_sha: project.commit.id, + after_sha: nil, + before_sha: nil, + source_sha: external_pull_request.source_sha, + target_sha: external_pull_request.target_sha, + trigger_request: nil, + schedule: nil, + external_pull_request: external_pull_request, + project: project, + current_user: user) + end + + let(:external_pull_request) { build(:external_pull_request, project: project) } + + before do + step.perform! + end + + it 'correctly indicated that this is an external pull request pipeline' do + expect(pipeline).to be_external_pull_request_event + expect(pipeline.external_pull_request).to eq(external_pull_request) + end + + it 'correctly sets source sha and target sha to pipeline' do + expect(pipeline.source_sha).to eq(external_pull_request.source_sha) + expect(pipeline.target_sha).to eq(external_pull_request.target_sha) + end + end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 47ba7eff8ed..a71619315d5 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -126,6 +126,8 @@ merge_requests: - blocks_as_blockee - blocking_merge_requests - blocked_merge_requests +external_pull_requests: +- project merge_request_diff: - merge_request - merge_request_diff_commits @@ -155,6 +157,7 @@ ci_pipelines: - pipeline_schedule - merge_requests_as_head_pipeline - merge_request +- external_pull_request - deployments - environments - chat_data @@ -402,6 +405,7 @@ project: - merge_trains - designs - project_aliases +- external_pull_requests award_emoji: - awardable - user diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 516e62c4728..1fa216f34b5 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -270,6 +270,7 @@ Ci::Pipeline: - protected - iid - merge_request_id +- external_pull_request_id Ci::Stage: - id - name @@ -713,3 +714,16 @@ List: - updated_at - milestone_id - user_id +ExternalPullRequest: +- id +- created_at +- updated_at +- project_id +- pull_request_iid +- status +- source_branch +- target_branch +- source_repository +- target_repository +- source_sha +- target_sha diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 63ca383ac4b..146e479adef 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -20,6 +20,7 @@ describe Ci::Pipeline, :mailer do it { is_expected.to belong_to(:auto_canceled_by) } it { is_expected.to belong_to(:pipeline_schedule) } it { is_expected.to belong_to(:merge_request) } + it { is_expected.to belong_to(:external_pull_request) } it { is_expected.to have_many(:statuses) } it { is_expected.to have_many(:trigger_requests) } @@ -885,6 +886,25 @@ describe Ci::Pipeline, :mailer do end end end + + context 'when source is external pull request' do + let(:pipeline) do + create(:ci_pipeline, source: :external_pull_request_event, external_pull_request: pull_request) + end + + let(:pull_request) { create(:external_pull_request, project: project) } + + it 'exposes external pull request pipeline variables' do + expect(subject.to_hash) + .to include( + 'CI_EXTERNAL_PULL_REQUEST_IID' => pull_request.pull_request_iid.to_s, + 'CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_SHA' => pull_request.source_sha, + 'CI_EXTERNAL_PULL_REQUEST_TARGET_BRANCH_SHA' => pull_request.target_sha, + 'CI_EXTERNAL_PULL_REQUEST_SOURCE_BRANCH_NAME' => pull_request.source_branch, + 'CI_EXTERNAL_PULL_REQUEST_TARGET_BRANCH_NAME' => pull_request.target_branch + ) + end + end end describe '#protected_ref?' do diff --git a/spec/models/external_pull_request_spec.rb b/spec/models/external_pull_request_spec.rb new file mode 100644 index 00000000000..e85d5b2f6c7 --- /dev/null +++ b/spec/models/external_pull_request_spec.rb @@ -0,0 +1,220 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ExternalPullRequest do + let(:project) { create(:project) } + let(:source_branch) { 'the-branch' } + let(:status) { :open } + + it { is_expected.to belong_to(:project) } + + shared_examples 'has errors on' do |attribute| + it "has errors for #{attribute}" do + expect(subject).not_to be_valid + expect(subject.errors[attribute]).not_to be_empty + end + end + + describe 'validations' do + context 'when source branch not present' do + subject { build(:external_pull_request, source_branch: nil) } + + it_behaves_like 'has errors on', :source_branch + end + + context 'when status not present' do + subject { build(:external_pull_request, status: nil) } + + it_behaves_like 'has errors on', :status + end + + context 'when pull request is from a fork' do + subject { build(:external_pull_request, source_repository: 'the-fork', target_repository: 'the-target') } + + it_behaves_like 'has errors on', :base + end + end + + describe 'create_or_update_from_params' do + subject { described_class.create_or_update_from_params(params) } + + context 'when pull request does not exist' do + context 'when params are correct' do + let(:params) do + { + project_id: project.id, + pull_request_iid: 123, + source_branch: 'feature', + target_branch: 'master', + source_repository: 'the-repository', + target_repository: 'the-repository', + source_sha: '97de212e80737a608d939f648d959671fb0a0142', + target_sha: 'a09386439ca39abe575675ffd4b89ae824fec22f', + status: :open + } + end + + it 'saves the model successfully and returns it' do + expect(subject).to be_persisted + expect(subject).to be_valid + end + + it 'yields the model' do + yielded_value = nil + + result = described_class.create_or_update_from_params(params) do |pull_request| + yielded_value = pull_request + end + + expect(result).to eq(yielded_value) + end + end + + context 'when params are not correct' do + let(:params) do + { + pull_request_iid: 123, + source_branch: 'feature', + target_branch: 'master', + source_repository: 'the-repository', + target_repository: 'the-repository', + source_sha: nil, + target_sha: nil, + status: :open + } + end + + it 'returns an invalid model' do + expect(subject).not_to be_persisted + expect(subject).not_to be_valid + end + end + end + + context 'when pull request exists' do + let!(:pull_request) do + create(:external_pull_request, + project: project, + source_sha: '97de212e80737a608d939f648d959671fb0a0142') + end + + context 'when params are correct' do + let(:params) do + { + pull_request_iid: pull_request.pull_request_iid, + source_branch: pull_request.source_branch, + target_branch: pull_request.target_branch, + source_repository: 'the-repository', + target_repository: 'the-repository', + source_sha: 'ce84140e8b878ce6e7c4d298c7202ff38170e3ac', + target_sha: pull_request.target_sha, + status: :open + } + end + + it 'updates the model successfully and returns it' do + expect(subject).to be_valid + expect(subject.source_sha).to eq(params[:source_sha]) + expect(pull_request.reload.source_sha).to eq(params[:source_sha]) + end + end + + context 'when params are not correct' do + let(:params) do + { + pull_request_iid: pull_request.pull_request_iid, + source_branch: pull_request.source_branch, + target_branch: pull_request.target_branch, + source_repository: 'the-repository', + target_repository: 'the-repository', + source_sha: nil, + target_sha: nil, + status: :open + } + end + + it 'returns an invalid model' do + expect(subject).not_to be_valid + expect(pull_request.reload.source_sha).not_to be_nil + expect(pull_request.target_sha).not_to be_nil + end + end + end + end + + describe '#open?' do + it 'returns true if status is open' do + pull_request = create(:external_pull_request, status: :open) + + expect(pull_request).to be_open + end + + it 'returns false if status is not open' do + pull_request = create(:external_pull_request, status: :closed) + + expect(pull_request).not_to be_open + end + end + + describe '#closed?' do + it 'returns true if status is closed' do + pull_request = build(:external_pull_request, status: :closed) + + expect(pull_request).to be_closed + end + + it 'returns false if status is not closed' do + pull_request = build(:external_pull_request, status: :open) + + expect(pull_request).not_to be_closed + end + end + + describe '#actual_branch_head?' do + let(:project) { create(:project, :repository) } + let(:branch) { project.repository.branches.first } + let(:source_branch) { branch.name } + + let(:pull_request) do + create(:external_pull_request, + project: project, + source_branch: source_branch, + source_sha: source_sha) + end + + context 'when source sha matches the head of the branch' do + let(:source_sha) { branch.target } + + it 'returns true' do + expect(pull_request).to be_actual_branch_head + end + end + + context 'when source sha does not match the head of the branch' do + let(:source_sha) { project.repository.commit('HEAD').sha } + + it 'returns true' do + expect(pull_request).not_to be_actual_branch_head + end + end + end + + describe '#from_fork?' do + it 'returns true if source_repository differs from target_repository' do + pull_request = build(:external_pull_request, + source_repository: 'repository-1', + target_repository: 'repository-2') + + expect(pull_request).to be_from_fork + end + + it 'returns false if source_repository is the same as target_repository' do + pull_request = build(:external_pull_request, + source_repository: 'repository-1', + target_repository: 'repository-1') + + expect(pull_request).not_to be_from_fork + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index bfbcac60fea..e2a684c42ae 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -99,6 +99,7 @@ describe Project do it { is_expected.to have_many(:project_deploy_tokens) } it { is_expected.to have_many(:deploy_tokens).through(:project_deploy_tokens) } it { is_expected.to have_many(:cycle_analytics_stages) } + it { is_expected.to have_many(:external_pull_requests) } it 'has an inverse relationship with merge requests' do expect(described_class.reflect_on_association(:merge_requests).has_inverse?).to eq(:target_project) diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index fad865a4811..50d1b9c3a97 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -23,6 +23,7 @@ describe Ci::CreatePipelineService do trigger_request: nil, variables_attributes: nil, merge_request: nil, + external_pull_request: nil, push_options: nil, source_sha: nil, target_sha: nil, @@ -36,8 +37,11 @@ describe Ci::CreatePipelineService do source_sha: source_sha, target_sha: target_sha } - described_class.new(project, user, params).execute( - source, save_on_errors: save_on_errors, trigger_request: trigger_request, merge_request: merge_request) + described_class.new(project, user, params).execute(source, + save_on_errors: save_on_errors, + trigger_request: trigger_request, + merge_request: merge_request, + external_pull_request: external_pull_request) end # rubocop:enable Metrics/ParameterLists @@ -773,6 +777,152 @@ describe Ci::CreatePipelineService do end end + describe 'Pipeline for external pull requests' do + let(:pipeline) do + execute_service(source: source, + external_pull_request: pull_request, + ref: ref_name, + source_sha: source_sha, + target_sha: target_sha) + end + + before do + stub_ci_pipeline_yaml_file(YAML.dump(config)) + end + + let(:ref_name) { 'refs/heads/feature' } + let(:source_sha) { project.commit(ref_name).id } + let(:target_sha) { nil } + + context 'when source is external pull request' do + let(:source) { :external_pull_request_event } + + context 'when config has external_pull_requests keywords' do + let(:config) do + { + build: { + stage: 'build', + script: 'echo' + }, + test: { + stage: 'test', + script: 'echo', + only: ['external_pull_requests'] + }, + pages: { + stage: 'deploy', + script: 'echo', + except: ['external_pull_requests'] + } + } + end + + context 'when external pull request is specified' do + let(:pull_request) { create(:external_pull_request, project: project, source_branch: 'feature', target_branch: 'master') } + let(:ref_name) { pull_request.source_ref } + + it 'creates an external pull request pipeline' do + expect(pipeline).to be_persisted + expect(pipeline).to be_external_pull_request_event + expect(pipeline.external_pull_request).to eq(pull_request) + expect(pipeline.source_sha).to eq(source_sha) + expect(pipeline.builds.order(:stage_id) + .map(&:name)) + .to eq(%w[build test]) + end + + context 'when ref is tag' do + let(:ref_name) { 'refs/tags/v1.1.0' } + + it 'does not create an extrnal pull request pipeline' do + expect(pipeline).not_to be_persisted + expect(pipeline.errors[:tag]).to eq(["is not included in the list"]) + end + end + + context 'when pull request is created from fork' do + it 'does not create an external pull request pipeline' + end + + context "when there are no matched jobs" do + let(:config) do + { + test: { + stage: 'test', + script: 'echo', + except: ['external_pull_requests'] + } + } + end + + it 'does not create a detached merge request pipeline' do + expect(pipeline).not_to be_persisted + expect(pipeline.errors[:base]).to eq(["No stages / jobs for this pipeline."]) + end + end + end + + context 'when external pull request is not specified' do + let(:pull_request) { nil } + + it 'does not create an external pull request pipeline' do + expect(pipeline).not_to be_persisted + expect(pipeline.errors[:external_pull_request]).to eq(["can't be blank"]) + end + end + end + + context "when config does not have external_pull_requests keywords" do + let(:config) do + { + build: { + stage: 'build', + script: 'echo' + }, + test: { + stage: 'test', + script: 'echo' + }, + pages: { + stage: 'deploy', + script: 'echo' + } + } + end + + context 'when external pull request is specified' do + let(:pull_request) do + create(:external_pull_request, + project: project, + source_branch: Gitlab::Git.ref_name(ref_name), + target_branch: 'master') + end + + it 'creates an external pull request pipeline' do + expect(pipeline).to be_persisted + expect(pipeline).to be_external_pull_request_event + expect(pipeline.external_pull_request).to eq(pull_request) + expect(pipeline.source_sha).to eq(source_sha) + expect(pipeline.builds.order(:stage_id) + .map(&:name)) + .to eq(%w[build test pages]) + end + end + + context 'when external pull request is not specified' do + let(:pull_request) { nil } + + it 'does not create an external pull request pipeline' do + expect(pipeline).not_to be_persisted + + expect(pipeline.errors[:base]) + .to eq(['Failed to build the pipeline!']) + end + end + end + end + end + describe 'Pipelines for merge requests' do let(:pipeline) do execute_service(source: source, diff --git a/spec/services/external_pull_requests/create_pipeline_service_spec.rb b/spec/services/external_pull_requests/create_pipeline_service_spec.rb new file mode 100644 index 00000000000..a4da5b38b97 --- /dev/null +++ b/spec/services/external_pull_requests/create_pipeline_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ExternalPullRequests::CreatePipelineService do + describe '#execute' do + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + let(:pull_request) { create(:external_pull_request, project: project) } + + before do + project.add_maintainer(user) + end + + subject { described_class.new(project, user).execute(pull_request) } + + context 'when pull request is open' do + before do + pull_request.update!(status: :open) + end + + context 'when source sha is the head of the source branch' do + let(:source_branch) { project.repository.branches.last } + let(:create_pipeline_service) { instance_double(Ci::CreatePipelineService) } + + before do + pull_request.update!(source_branch: source_branch.name, source_sha: source_branch.target) + end + + it 'creates a pipeline for external pull request' do + expect(subject).to be_valid + expect(subject).to be_persisted + expect(subject).to be_external_pull_request_event + expect(subject).to eq(project.ci_pipelines.last) + expect(subject.external_pull_request).to eq(pull_request) + expect(subject.user).to eq(user) + expect(subject.status).to eq('pending') + expect(subject.ref).to eq(pull_request.source_branch) + expect(subject.sha).to eq(pull_request.source_sha) + expect(subject.source_sha).to eq(pull_request.source_sha) + end + end + + context 'when source sha is not the head of the source branch (force push upon rebase)' do + let(:source_branch) { project.repository.branches.first } + let(:commit) { project.repository.commits(source_branch.name, limit: 2).last } + + before do + pull_request.update!(source_branch: source_branch.name, source_sha: commit.sha) + end + + it 'does nothing' do + expect(Ci::CreatePipelineService).not_to receive(:new) + + expect(subject).to be_nil + end + end + end + + context 'when pull request is not opened' do + before do + pull_request.update!(status: :closed) + end + + it 'does nothing' do + expect(Ci::CreatePipelineService).not_to receive(:new) + + expect(subject).to be_nil + end + end + end +end diff --git a/spec/workers/update_external_pull_requests_worker_spec.rb b/spec/workers/update_external_pull_requests_worker_spec.rb new file mode 100644 index 00000000000..f3956bb3514 --- /dev/null +++ b/spec/workers/update_external_pull_requests_worker_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe UpdateExternalPullRequestsWorker do + describe '#perform' do + set(:project) { create(:project, import_source: 'tanuki/repository') } + set(:user) { create(:user) } + let(:worker) { described_class.new } + + before do + create(:external_pull_request, + project: project, + source_repository: project.import_source, + target_repository: project.import_source, + source_branch: 'feature-1', + target_branch: 'master') + + create(:external_pull_request, + project: project, + source_repository: project.import_source, + target_repository: project.import_source, + source_branch: 'feature-1', + target_branch: 'develop') + end + + subject { worker.perform(project.id, user.id, ref) } + + context 'when ref is a branch' do + let(:ref) { 'refs/heads/feature-1' } + let(:create_pipeline_service) { instance_double(ExternalPullRequests::CreatePipelineService) } + + it 'runs CreatePipelineService for each pull request matching the source branch and repository' do + expect(ExternalPullRequests::CreatePipelineService) + .to receive(:new) + .and_return(create_pipeline_service) + .twice + expect(create_pipeline_service).to receive(:execute).twice + + subject + end + end + + context 'when ref is not a branch' do + let(:ref) { 'refs/tags/v1.2.3' } + + it 'does nothing' do + expect(ExternalPullRequests::CreatePipelineService).not_to receive(:new) + + subject + end + end + end +end -- cgit v1.2.3