From 040aac148d1aa8a9b361164311e54a56be2f5d42 Mon Sep 17 00:00:00 2001 From: Mathieu Parent Date: Wed, 18 Mar 2020 10:38:39 +0100 Subject: [PATCH 1/2] Add setting to allow merge on skipped pipeline Fixes: #211482 --- app/controllers/projects_controller.rb | 1 + app/graphql/types/project_type.rb | 2 + app/models/merge_request.rb | 1 + app/models/project.rb | 7 +- app/models/project_setting.rb | 4 - ...ge_request_merge_checks_settings.html.haml | 11 ++- changelogs/unreleased/allow_skipped.yml | 5 ++ ...on_skipped_pipeline_to_project_settings.rb | 9 +++ db/structure.sql | 2 + .../graphql/reference/gitlab_schema.graphql | 6 ++ doc/api/graphql/reference/gitlab_schema.json | 14 ++++ doc/api/graphql/reference/index.md | 1 + doc/api/projects.md | 15 ++++ .../merge_when_pipeline_succeeds.md | 13 ++++ lib/api/entities/project.rb | 2 + lib/api/helpers/projects_helpers.rb | 2 + locale/gitlab.pot | 6 ++ spec/lib/gitlab/import_export/all_models.yml | 1 + .../import_export/safe_model_attributes.yml | 2 + spec/models/merge_request_spec.rb | 73 +++++++++++++++++-- spec/models/project_spec.rb | 4 +- spec/requests/api/projects_spec.rb | 41 ++++++++++- spec/services/projects/create_service_spec.rb | 4 +- 23 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/allow_skipped.yml create mode 100644 db/migrate/20200325094612_add_allow_merge_on_skipped_pipeline_to_project_settings.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 02c84ce33d8c52..f0ddd62e9964dc 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -362,6 +362,7 @@ def project_params(attributes: []) def project_params_attributes [ + :allow_merge_on_skipped_pipeline, :avatar, :build_allow_git_fetch, :build_coverage_regex, diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index 88a9adfc393499..f0e2e4c7ae335d 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -95,6 +95,8 @@ class ProjectType < BaseObject description: 'Status of Jira import background job of the project' field :only_allow_merge_if_pipeline_succeeds, GraphQL::BOOLEAN_TYPE, null: true, description: 'Indicates if merge requests of the project can only be merged with successful jobs' + field :allow_merge_on_skipped_pipeline, GraphQL::BOOLEAN_TYPE, null: true, + description: 'If `only_allow_merge_if_pipeline_succeeds` is true, indicates if merge requests of the project can also be merged with skipped jobs' field :request_access_enabled, GraphQL::BOOLEAN_TYPE, null: true, description: 'Indicates if users can request member access to the project' field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true, diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1453b1f11d234b..caf7b554427dac 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1169,6 +1169,7 @@ def can_be_merged_via_command_line_by?(user) def mergeable_ci_state? return true unless project.only_allow_merge_if_pipeline_succeeds? return false unless actual_head_pipeline + return true if project.allow_merge_on_skipped_pipeline? && actual_head_pipeline.skipped? actual_head_pipeline.success? end diff --git a/app/models/project.rb b/app/models/project.rb index a70f016a9f3a53..bdcef14d705d07 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -198,7 +198,7 @@ class Project < ApplicationRecord has_one :error_tracking_setting, inverse_of: :project, class_name: 'ErrorTracking::ProjectErrorTrackingSetting' has_one :metrics_setting, inverse_of: :project, class_name: 'ProjectMetricsSetting' has_one :grafana_integration, inverse_of: :project - has_one :project_setting, ->(project) { where_or_create_by(project: project) }, inverse_of: :project + has_one :project_setting, inverse_of: :project, autosave: true has_one :alerting_setting, inverse_of: :project, class_name: 'Alerting::ProjectAlertingSetting' # Merge Requests for target project should be removed with it @@ -376,6 +376,7 @@ class Project < ApplicationRecord delegate :default_git_depth, :default_git_depth=, to: :ci_cd_settings, prefix: :ci delegate :forward_deployment_enabled, :forward_deployment_enabled=, :forward_deployment_enabled?, to: :ci_cd_settings delegate :actual_limits, :actual_plan_name, to: :namespace, allow_nil: true + delegate :allow_merge_on_skipped_pipeline, :allow_merge_on_skipped_pipeline?, :allow_merge_on_skipped_pipeline=, to: :project_setting # Validations validates :creator, presence: true, on: :create @@ -727,6 +728,10 @@ def initialize(attributes = nil) super end + def project_setting + super.presence || build_project_setting + end + def all_pipelines if builds_enabled? super diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 7c93faf3928be2..9022d3e879d0c4 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -4,10 +4,6 @@ class ProjectSetting < ApplicationRecord belongs_to :project, inverse_of: :project_setting self.primary_key = :project_id - - def self.where_or_create_by(attrs) - where(primary_key => safe_find_or_create_by(attrs)) - end end ProjectSetting.prepend_if_ee('EE::ProjectSetting') diff --git a/app/views/projects/_merge_request_merge_checks_settings.html.haml b/app/views/projects/_merge_request_merge_checks_settings.html.haml index d3fcb52422b1eb..cd8bf4d06da58d 100644 --- a/app/views/projects/_merge_request_merge_checks_settings.html.haml +++ b/app/views/projects/_merge_request_merge_checks_settings.html.haml @@ -1,9 +1,11 @@ - form = local_assigns.fetch(:form) +- project = local_assigns.fetch(:project) +- skipped_pipeline_id = 'js-skipped-pipeline-allowed-container' .form-group %b= s_('ProjectSettings|Merge checks') %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged') - .form-check.mb-2.builds-feature + .form-check.mb-2.builds-feature{ data: { toggle: 'collapse', target: "##{skipped_pipeline_id}" } } = form.check_box :only_allow_merge_if_pipeline_succeeds, class: 'form-check-input' = form.label :only_allow_merge_if_pipeline_succeeds, class: 'form-check-label' do = s_('ProjectSettings|Pipelines must succeed') @@ -13,6 +15,13 @@ help_page_path('ci/merge_request_pipelines/index.md', anchor: 'pipelines-for-merge-requests'), target: '_blank' + .form-check.mb-2.collapse{ id: skipped_pipeline_id, class: project.only_allow_merge_if_pipeline_succeeds ? 'show' : '' } + .gl-pl-6 + = form.check_box :allow_merge_on_skipped_pipeline, class: 'form-check-input' + = form.label :allow_merge_on_skipped_pipeline, class: 'form-check-label' do + = s_('ProjectSettings|Skipped pipelines are considered successful') + .text-secondary + = s_('ProjectSettings|This introduces the risk of merging changes that will not pass the pipeline.') .form-check.mb-2 = form.check_box :only_allow_merge_if_all_discussions_are_resolved, class: 'form-check-input' = form.label :only_allow_merge_if_all_discussions_are_resolved, class: 'form-check-label' do diff --git a/changelogs/unreleased/allow_skipped.yml b/changelogs/unreleased/allow_skipped.yml new file mode 100644 index 00000000000000..b920298f42e24c --- /dev/null +++ b/changelogs/unreleased/allow_skipped.yml @@ -0,0 +1,5 @@ +--- +title: Add setting to allow merge on skipped pipeline +merge_request: 27490 +author: Mathieu Parent +type: added diff --git a/db/migrate/20200325094612_add_allow_merge_on_skipped_pipeline_to_project_settings.rb b/db/migrate/20200325094612_add_allow_merge_on_skipped_pipeline_to_project_settings.rb new file mode 100644 index 00000000000000..8575dd2f080d27 --- /dev/null +++ b/db/migrate/20200325094612_add_allow_merge_on_skipped_pipeline_to_project_settings.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddAllowMergeOnSkippedPipelineToProjectSettings < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :project_settings, :allow_merge_on_skipped_pipeline, :boolean + end +end diff --git a/db/structure.sql b/db/structure.sql index 0018eeb4eaba81..a7ed83620cdd32 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -5340,6 +5340,7 @@ CREATE TABLE public.project_settings ( updated_at timestamp with time zone NOT NULL, push_rule_id bigint, show_default_award_emojis boolean DEFAULT true, + allow_merge_on_skipped_pipeline boolean, CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL)) ); @@ -13613,6 +13614,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200323134519 20200324093258 20200324115359 +20200325094612 20200325104755 20200325104756 20200325104833 diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index b01af67204955d..cbe79658f38584 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -8265,6 +8265,12 @@ type Project { statuses: [AlertManagementStatus!] ): AlertManagementAlertConnection + """ + If `only_allow_merge_if_pipeline_succeeds` is true, indicates if merge + requests of the project can also be merged with skipped jobs + """ + allowMergeOnSkippedPipeline: Boolean + """ Indicates the archived status of the project """ diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index c5d172c035d9c0..40dad1852897a7 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -24673,6 +24673,20 @@ "isDeprecated": false, "deprecationReason": null }, + { + "name": "allowMergeOnSkippedPipeline", + "description": "If `only_allow_merge_if_pipeline_succeeds` is true, indicates if merge requests of the project can also be merged with skipped jobs", + "args": [ + + ], + "type": { + "kind": "SCALAR", + "name": "Boolean", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, { "name": "archived", "description": "Indicates the archived status of the project", diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index a3f84642e134ff..6f404e47fb4266 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -1252,6 +1252,7 @@ Information about pagination in a connection. | --- | ---- | ---------- | | `alertManagementAlert` | AlertManagementAlert | A single Alert Management alert of the project | | `alertManagementAlertStatusCounts` | AlertManagementAlertStatusCountsType | Counts of alerts by status for the project | +| `allowMergeOnSkippedPipeline` | Boolean | If `only_allow_merge_if_pipeline_succeeds` is true, indicates if merge requests of the project can also be merged with skipped jobs | | `archived` | Boolean | Indicates the archived status of the project | | `autocloseReferencedIssues` | Boolean | Indicates if issues referenced by merge requests and commits within the default branch are closed automatically | | `avatarUrl` | String | URL to avatar image file of the project | diff --git a/doc/api/projects.md b/doc/api/projects.md index ed6f9360f9ac26..b9ba632cd9e427 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -158,6 +158,7 @@ When the user is authenticated and `simple` is not set this returns something li "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -248,6 +249,7 @@ When the user is authenticated and `simple` is not set this returns something li "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -407,6 +409,7 @@ This endpoint supports [keyset pagination](README.md#keyset-based-pagination) fo "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -497,6 +500,7 @@ This endpoint supports [keyset pagination](README.md#keyset-based-pagination) fo "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -623,6 +627,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -708,6 +713,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -861,6 +867,7 @@ GET /projects/:id ], "repository_storage": "default", "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "printing_merge_requests_link_enabled": true, @@ -1044,6 +1051,7 @@ POST /projects | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, jobs can be viewed by non-project-members | | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs | +| `allow_merge_on_skipped_pipeline` | boolean | no | Set whether or not merge requests can be merged with skipped jobs | | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `merge_method` | string | no | Set the [merge method](#project-merge-method) used | | `autoclose_referenced_issues` | boolean | no | Set whether auto-closing referenced issues on default branch | @@ -1113,6 +1121,7 @@ POST /projects/user/:user_id | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, jobs can be viewed by non-project-members | | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs | +| `allow_merge_on_skipped_pipeline` | boolean | no | Set whether or not merge requests can be merged with skipped jobs | | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `merge_method` | string | no | Set the [merge method](#project-merge-method) used | | `autoclose_referenced_issues` | boolean | no | Set whether auto-closing referenced issues on default branch | @@ -1183,6 +1192,7 @@ PUT /projects/:id | `import_url` | string | no | URL to import repository from | | `public_builds` | boolean | no | If `true`, jobs can be viewed by non-project-members | | `only_allow_merge_if_pipeline_succeeds` | boolean | no | Set whether merge requests can only be merged with successful jobs | +| `allow_merge_on_skipped_pipeline` | boolean | no | Set whether or not merge requests can be merged with skipped jobs | | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved | | `merge_method` | string | no | Set the [merge method](#project-merge-method) used | | `autoclose_referenced_issues` | boolean | no | Set whether auto-closing referenced issues on default branch | @@ -1317,6 +1327,7 @@ Example responses: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -1408,6 +1419,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -1498,6 +1510,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -1680,6 +1693,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, @@ -1789,6 +1803,7 @@ Example response: "public_jobs": true, "shared_with_groups": [], "only_allow_merge_if_pipeline_succeeds": false, + "allow_merge_on_skipped_pipeline": false, "only_allow_merge_if_all_discussions_are_resolved": false, "remove_source_branch_after_merge": false, "request_access_enabled": false, diff --git a/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md b/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md index 5eedd0cf8a763f..d45ccdc9be9d6e 100644 --- a/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md +++ b/doc/user/project/merge_requests/merge_when_pipeline_succeeds.md @@ -59,6 +59,19 @@ merge request from the UI, until you make all relevant jobs pass. ![Only allow merge if pipeline succeeds message](img/merge_when_pipeline_succeeds_only_if_succeeds_msg.png) +### Skipped pipelines + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/211482) in GitLab 13.1. + +When the **Pipelines must succeed** checkbox is checked, [skipped pipelines](../../../ci/yaml/README.md#skip-pipeline) prevent +merge requests from being merged. To change this behavior: + +1. Navigate to your project's **Settings > General** page. +1. Expand the **Merge requests** section. +1. In the **Merge checks** subsection, ensure **Pipelines must succeed** is checked. +1. In the **Merge checks** subsection, select the **Skipped pipelines are considered successful** checkbox. +1. Press **Save** for the changes to take effect. + ### Limitations When this setting is enabled, a merge request is prevented from being merged if there is no pipeline. This may conflict with some use cases where [`only/except`](../../../ci/yaml/README.md#onlyexcept-advanced) rules are used and they don't generate any pipelines. diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index b6fa419da58fc4..55a57501858b28 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -93,6 +93,7 @@ class Project < BasicProjectDetails SharedGroupWithProject.represent(project.project_group_links, options) end expose :only_allow_merge_if_pipeline_succeeds + expose :allow_merge_on_skipped_pipeline expose :request_access_enabled expose :only_allow_merge_if_all_discussions_are_resolved expose :remove_source_branch_after_merge @@ -119,6 +120,7 @@ def self.preload_relation(projects_relation, options = {}) # MR describing the solution: https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/20555 super(projects_relation).preload(:group) .preload(:ci_cd_settings) + .preload(:project_setting) .preload(:container_expiration_policy) .preload(:auto_devops) .preload(project_group_links: { group: :route }, diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb index 5afdb34da97154..8a115d42929c69 100644 --- a/lib/api/helpers/projects_helpers.rb +++ b/lib/api/helpers/projects_helpers.rb @@ -44,6 +44,7 @@ module ProjectsHelpers optional :public_builds, type: Boolean, desc: 'Perform public builds' optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed' + optional :allow_merge_on_skipped_pipeline, type: Boolean, desc: 'Allow to merge if pipeline is skipped' optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved' optional :tag_list, type: Array[String], desc: 'The list of tags for a project' # TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab/issues/14960 @@ -92,6 +93,7 @@ module ProjectsHelpers def self.update_params_at_least_one_of [ + :allow_merge_on_skipped_pipeline, :autoclose_referenced_issues, :auto_devops_enabled, :auto_devops_deploy_strategy, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 210f51a73dbf45..a1b3105fcc2b97 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17341,6 +17341,9 @@ msgstr "" msgid "ProjectSettings|Show link to create/view merge request when pushing from the command line" msgstr "" +msgid "ProjectSettings|Skipped pipelines are considered successful" +msgstr "" + msgid "ProjectSettings|Snippets" msgstr "" @@ -17356,6 +17359,9 @@ msgstr "" msgid "ProjectSettings|These checks must pass before merge requests can be merged" msgstr "" +msgid "ProjectSettings|This introduces the risk of merging changes that will not pass the pipeline." +msgstr "" + msgid "ProjectSettings|This setting is applied on the server level and can be overridden by an admin." msgstr "" diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 235d54e3a16fe3..849d8fcc1c42c0 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -424,6 +424,7 @@ project: - deploy_tokens - settings - ci_cd_settings +- project_settings - import_export_upload - repository_languages - pool_repository diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 97fd30837d44c0..0d112bfdb2a20f 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -703,6 +703,8 @@ Badge: - type ProjectCiCdSetting: - group_runners_enabled +ProjectSetting: +- allow_merge_on_skipped_pipeline ProtectedEnvironment: - id - project_id diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 8c50966078f4c8..c70ddac5da6bbf 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2517,12 +2517,13 @@ def set_compare(merge_request) end describe '#mergeable_ci_state?' do - let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } let(:pipeline) { create(:ci_empty_pipeline) } - subject { build(:merge_request, target_project: project) } - context 'when it is only allowed to merge when build is green' do + let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } + + subject { build(:merge_request, target_project: project) } + context 'and a failed pipeline is associated' do before do pipeline.update(status: 'failed', sha: subject.diff_head_sha) @@ -2544,7 +2545,7 @@ def set_compare(merge_request) context 'and a skipped pipeline is associated' do before do pipeline.update(status: 'skipped', sha: subject.diff_head_sha) - allow(subject).to receive(:head_pipeline) { pipeline } + allow(subject).to receive(:head_pipeline).and_return(pipeline) end it { expect(subject.mergeable_ci_state?).to be_falsey } @@ -2552,7 +2553,48 @@ def set_compare(merge_request) context 'when no pipeline is associated' do before do - allow(subject).to receive(:head_pipeline) { nil } + allow(subject).to receive(:head_pipeline).and_return(nil) + end + + it { expect(subject.mergeable_ci_state?).to be_falsey } + end + end + + context 'when it is only allowed to merge when build is green or skipped' do + let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true, allow_merge_on_skipped_pipeline: true) } + + subject { build(:merge_request, target_project: project) } + + context 'and a failed pipeline is associated' do + before do + pipeline.update!(status: 'failed', sha: subject.diff_head_sha) + allow(subject).to receive(:head_pipeline).and_return(pipeline) + end + + it { expect(subject.mergeable_ci_state?).to be_falsey } + end + + context 'and a successful pipeline is associated' do + before do + pipeline.update!(status: 'success', sha: subject.diff_head_sha) + allow(subject).to receive(:head_pipeline).and_return(pipeline) + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + + context 'and a skipped pipeline is associated' do + before do + pipeline.update!(status: 'skipped', sha: subject.diff_head_sha) + allow(subject).to receive(:head_pipeline).and_return(pipeline) + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + + context 'when no pipeline is associated' do + before do + allow(subject).to receive(:head_pipeline).and_return(nil) end it { expect(subject.mergeable_ci_state?).to be_falsey } @@ -2560,7 +2602,9 @@ def set_compare(merge_request) end context 'when merges are not restricted to green builds' do - subject { build(:merge_request, target_project: create(:project, only_allow_merge_if_pipeline_succeeds: false)) } + let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: false) } + + subject { build(:merge_request, target_project: project) } context 'and a failed pipeline is associated' do before do @@ -2578,6 +2622,23 @@ def set_compare(merge_request) it { expect(subject.mergeable_ci_state?).to be_truthy } end + + context 'and a skipped pipeline is associated' do + before do + pipeline.update!(status: 'skipped', sha: subject.diff_head_sha) + allow(subject).to receive(:head_pipeline).and_return(pipeline) + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end + + context 'when no pipeline is associated' do + before do + allow(subject).to receive(:head_pipeline).and_return(nil) + end + + it { expect(subject.mergeable_ci_state?).to be_truthy } + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 787d778b483bf9..1a19f6f8814c04 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -183,9 +183,9 @@ expect(project.pages_metadatum).to be_persisted end - it 'automatically creates a project setting row' do + it 'automatically builds a project setting row' do expect(project.project_setting).to be_an_instance_of(ProjectSetting) - expect(project.project_setting).to be_persisted + expect(project.project_setting).to be_new_record end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 6f6737864b6f89..5af4ad020fc356 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -764,7 +764,8 @@ resolve_outdated_diff_discussions: false, remove_source_branch_after_merge: true, autoclose_referenced_issues: true, - only_allow_merge_if_pipeline_succeeds: false, + only_allow_merge_if_pipeline_succeeds: true, + allow_merge_on_skipped_pipeline: true, request_access_enabled: true, only_allow_merge_if_all_discussions_are_resolved: false, ci_config_path: 'a/custom/path', @@ -912,6 +913,22 @@ expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_truthy end + it 'sets a project as not allowing merge when pipeline is skipped' do + project_params = attributes_for(:project, allow_merge_on_skipped_pipeline: false) + + post api('/projects', user), params: project_params + + expect(json_response['allow_merge_on_skipped_pipeline']).to be_falsey + end + + it 'sets a project as allowing merge when pipeline is skipped' do + project_params = attributes_for(:project, allow_merge_on_skipped_pipeline: true) + + post api('/projects', user), params: project_params + + expect(json_response['allow_merge_on_skipped_pipeline']).to be_truthy + end + it 'sets a project as allowing merge even if discussions are unresolved' do project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false) @@ -1245,16 +1262,36 @@ it 'sets a project as allowing merge even if build fails' do project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: false) + post api("/projects/user/#{user.id}", admin), params: project + expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_falsey end it 'sets a project as allowing merge only if pipeline succeeds' do project = attributes_for(:project, only_allow_merge_if_pipeline_succeeds: true) + post api("/projects/user/#{user.id}", admin), params: project + expect(json_response['only_allow_merge_if_pipeline_succeeds']).to be_truthy end + it 'sets a project as not allowing merge when pipeline is skipped' do + project = attributes_for(:project, allow_merge_on_skipped_pipeline: false) + + post api("/projects/user/#{user.id}", admin), params: project + + expect(json_response['allow_merge_on_skipped_pipeline']).to be_falsey + end + + it 'sets a project as allowing merge when pipeline is skipped' do + project = attributes_for(:project, allow_merge_on_skipped_pipeline: true) + + post api("/projects/user/#{user.id}", admin), params: project + + expect(json_response['allow_merge_on_skipped_pipeline']).to be_truthy + end + it 'sets a project as allowing merge even if discussions are unresolved' do project = attributes_for(:project, only_allow_merge_if_all_discussions_are_resolved: false) @@ -1394,6 +1431,7 @@ expect(json_response['shared_with_groups'][0]['group_name']).to eq(group.name) expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access) expect(json_response['only_allow_merge_if_pipeline_succeeds']).to eq(project.only_allow_merge_if_pipeline_succeeds) + expect(json_response['allow_merge_on_skipped_pipeline']).to eq(project.allow_merge_on_skipped_pipeline) expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved) end end @@ -1461,6 +1499,7 @@ expect(json_response['shared_with_groups'][0]['group_access_level']).to eq(link.group_access) expect(json_response['shared_with_groups'][0]).to have_key('expires_at') expect(json_response['only_allow_merge_if_pipeline_succeeds']).to eq(project.only_allow_merge_if_pipeline_succeeds) + expect(json_response['allow_merge_on_skipped_pipeline']).to eq(project.allow_merge_on_skipped_pipeline) expect(json_response['only_allow_merge_if_all_discussions_are_resolved']).to eq(project.only_allow_merge_if_all_discussions_are_resolved) expect(json_response['ci_default_git_depth']).to eq(project.ci_default_git_depth) expect(json_response['merge_method']).to eq(project.merge_method.to_s) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 729e5d9c23d5d3..e70ee05ed31a43 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -43,10 +43,10 @@ create_project(user, opts) end - it 'creates associated project settings' do + it 'builds associated project settings' do project = create_project(user, opts) - expect(project.project_setting).to be_persisted + expect(project.project_setting).to be_new_record end end -- GitLab From 470371c9f7595bfbb8e3a6b2a37e9af776b5d0b2 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sun, 7 Jun 2020 14:05:25 -0500 Subject: [PATCH 2/2] Revert bootstrap collapse on allow skipped check - This was unreliable and will be reevaluated in a follow-up --- .../projects/_merge_request_merge_checks_settings.html.haml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/views/projects/_merge_request_merge_checks_settings.html.haml b/app/views/projects/_merge_request_merge_checks_settings.html.haml index cd8bf4d06da58d..9cebb191346f80 100644 --- a/app/views/projects/_merge_request_merge_checks_settings.html.haml +++ b/app/views/projects/_merge_request_merge_checks_settings.html.haml @@ -1,11 +1,9 @@ - form = local_assigns.fetch(:form) -- project = local_assigns.fetch(:project) -- skipped_pipeline_id = 'js-skipped-pipeline-allowed-container' .form-group %b= s_('ProjectSettings|Merge checks') %p.text-secondary= s_('ProjectSettings|These checks must pass before merge requests can be merged') - .form-check.mb-2.builds-feature{ data: { toggle: 'collapse', target: "##{skipped_pipeline_id}" } } + .form-check.mb-2.builds-feature = form.check_box :only_allow_merge_if_pipeline_succeeds, class: 'form-check-input' = form.label :only_allow_merge_if_pipeline_succeeds, class: 'form-check-label' do = s_('ProjectSettings|Pipelines must succeed') @@ -15,7 +13,7 @@ help_page_path('ci/merge_request_pipelines/index.md', anchor: 'pipelines-for-merge-requests'), target: '_blank' - .form-check.mb-2.collapse{ id: skipped_pipeline_id, class: project.only_allow_merge_if_pipeline_succeeds ? 'show' : '' } + .form-check.mb-2 .gl-pl-6 = form.check_box :allow_merge_on_skipped_pipeline, class: 'form-check-input' = form.label :allow_merge_on_skipped_pipeline, class: 'form-check-label' do -- GitLab