From 01be4d0e450c5ac038b542b60337162feb2e53ab Mon Sep 17 00:00:00 2001
From: Tetiana Zavediuk <tzave@softserveinc.com>
Date: Thu, 13 Oct 2022 08:22:02 +0000
Subject: [PATCH] Map 'Require pull request before merging' GitHub rule with
access_levels
Select Developers + Maintainers in the Allowed to merge list, and select
'No one' in the 'Allowed to push list' in GitLab protected branch
settings when importing a GitHub project to GitLab if the "Require
a pull request before merging" GitHub protected rule is enabled.
Details: https://gitlab.com/gitlab-org/gitlab/-/issues/370951
Changelog: added
---
.../protected_branch/merge_access_level.rb | 2 +
.../protected_branch/push_access_level.rb | 2 +
doc/user/project/import/github.md | 2 +-
.../importer/protected_branch_importer.rb | 73 ++++++++++-
.../representation/protected_branch.rb | 8 +-
.../protected_branch_importer_spec.rb | 115 +++++++++++++++++-
.../protected_branches_importer_spec.rb | 6 +
.../representation/protected_branch_spec.rb | 13 +-
8 files changed, 210 insertions(+), 11 deletions(-)
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index de240e4031630..df75c557717a9 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -2,4 +2,6 @@
class ProtectedBranch::MergeAccessLevel < ApplicationRecord
include ProtectedBranchAccess
+ # default value for the access_level column
+ GITLAB_DEFAULT_ACCESS_LEVEL = Gitlab::Access::MAINTAINER
end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 5248834a2f229..6076fab20b7c7 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -2,6 +2,8 @@
class ProtectedBranch::PushAccessLevel < ApplicationRecord
include ProtectedBranchAccess
+ # default value for the access_level column
+ GITLAB_DEFAULT_ACCESS_LEVEL = Gitlab::Access::MAINTAINER
belongs_to :deploy_key
diff --git a/doc/user/project/import/github.md b/doc/user/project/import/github.md
index 0e556de515f88..0fc49798a1511 100644
--- a/doc/user/project/import/github.md
+++ b/doc/user/project/import/github.md
@@ -204,7 +204,7 @@ defaults to the default project visibility.
Supported GitHub branch protection rules are mapped to GitLab branch protection rules or project-wide GitLab settings when they are imported:
- GitHub rule **Require conversation resolution before merging** for the project's default branch is mapped to the [**All threads must be resolved** GitLab setting](../../discussions/index.md#prevent-merge-unless-all-threads-are-resolved). [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/371110) in GitLab 15.5.
-- Support for GitHub rule **Require a pull request before merging** is proposed in issue [370951](https://gitlab.com/gitlab-org/gitlab/-/issues/370951).
+- GitHub rule **Require a pull request before merging** is mapped to the **No one** option in the **Allowed to push** list of the branch protection rule. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370951) in GitLab 15.5.
- GitHub rule **Require signed commits** for the project's default branch is mapped to the **Reject unsigned commits** GitLab setting. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/370949) in GitLab 15.5.
- Support for GitHub rule **Require status checks to pass before merging** was proposed in issue [370948](https://gitlab.com/gitlab-org/gitlab/-/issues/370948). However, this rule cannot be translated during project import into GitLab due to technical difficulties.
You can still create [status checks](../merge_requests/status_checks.md) in GitLab yourself.
diff --git a/lib/gitlab/github_import/importer/protected_branch_importer.rb b/lib/gitlab/github_import/importer/protected_branch_importer.rb
index ac80f40c8bc7e..21075e21e1def 100644
--- a/lib/gitlab/github_import/importer/protected_branch_importer.rb
+++ b/lib/gitlab/github_import/importer/protected_branch_importer.rb
@@ -6,6 +6,10 @@ module Importer
class ProtectedBranchImporter
attr_reader :protected_branch, :project, :client
+ # By default on GitHub, both developers and maintainers can merge
+ # a PR into the protected branch
+ GITHUB_DEFAULT_MERGE_ACCESS_LEVEL = Gitlab::Access::DEVELOPER
+
# protected_branch - An instance of
# `Gitlab::GithubImport::Representation::ProtectedBranch`.
# project - An instance of `Project`
@@ -31,8 +35,8 @@ def execute
def params
{
name: protected_branch.id,
- push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
- merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
+ push_access_levels_attributes: [{ access_level: push_access_level }],
+ merge_access_levels_attributes: [{ access_level: merge_access_level }],
allow_force_push: allow_force_push?
}
end
@@ -68,6 +72,71 @@ def update_project_push_rule
push_rule.update!(reject_unsigned_commits: true)
project.project_setting.update!(push_rule_id: push_rule.id)
end
+
+ def push_access_level
+ if protected_branch.required_pull_request_reviews
+ Gitlab::Access::NO_ACCESS
+ else
+ gitlab_access_level_for(:push)
+ end
+ end
+
+ # Gets the strictest merge_access_level between GitHub and GitLab
+ def merge_access_level
+ gitlab_access = gitlab_access_level_for(:merge)
+
+ return gitlab_access if gitlab_access == Gitlab::Access::NO_ACCESS
+
+ [gitlab_access, GITHUB_DEFAULT_MERGE_ACCESS_LEVEL].max
+ end
+
+ # action - :push/:merge
+ def gitlab_access_level_for(action)
+ if default_branch?
+ action == :push ? default_branch_push_access_level : default_branch_merge_access_level
+ elsif protected_on_gitlab?
+ non_default_branch_access_level_for(action)
+ else
+ gitlab_default_access_level_for(action)
+ end
+ end
+
+ def default_branch_push_access_level
+ if default_branch_protection.developer_can_push?
+ Gitlab::Access::DEVELOPER
+ else
+ gitlab_default_access_level_for(:push)
+ end
+ end
+
+ def default_branch_merge_access_level
+ if default_branch_protection.developer_can_merge?
+ Gitlab::Access::DEVELOPER
+ else
+ gitlab_default_access_level_for(:merge)
+ end
+ end
+
+ def default_branch_protection
+ Gitlab::Access::BranchProtection.new(project.namespace.default_branch_protection)
+ end
+
+ def protected_on_gitlab?
+ ProtectedBranch.protected?(project, protected_branch.id)
+ end
+
+ def non_default_branch_access_level_for(action)
+ access_level = ProtectedBranch.access_levels_for_ref(protected_branch.id, action: action)
+ .find(&:role?)&.access_level
+
+ access_level || gitlab_default_access_level_for(action)
+ end
+
+ def gitlab_default_access_level_for(action)
+ return ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL if action == :push
+
+ ProtectedBranch::MergeAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL
+ end
end
end
end
diff --git a/lib/gitlab/github_import/representation/protected_branch.rb b/lib/gitlab/github_import/representation/protected_branch.rb
index 4795e67e01152..07a607ae70dfa 100644
--- a/lib/gitlab/github_import/representation/protected_branch.rb
+++ b/lib/gitlab/github_import/representation/protected_branch.rb
@@ -9,12 +9,13 @@ class ProtectedBranch
attr_reader :attributes
- expose_attribute :id, :allow_force_pushes, :required_conversation_resolution, :required_signatures
+ expose_attribute :id, :allow_force_pushes, :required_conversation_resolution, :required_signatures,
+ :required_pull_request_reviews
# Builds a Branch Protection info from a GitHub API response.
# Resource structure details:
# https://docs.github.com/en/rest/branches/branch-protection#get-branch-protection
- # branch_protection - An instance of `Sawyer::Resource` containing the protection details.
+ # branch_protection - An instance of `Hash` containing the protection details.
def self.from_api_response(branch_protection, _additional_object_data = {})
branch_name = branch_protection[:url].match(%r{/branches/(\S{1,255})/protection$})[1]
@@ -22,7 +23,8 @@ def self.from_api_response(branch_protection, _additional_object_data = {})
id: branch_name,
allow_force_pushes: branch_protection.dig(:allow_force_pushes, :enabled),
required_conversation_resolution: branch_protection.dig(:required_conversation_resolution, :enabled),
- required_signatures: branch_protection.dig(:required_signatures, :enabled)
+ required_signatures: branch_protection.dig(:required_signatures, :enabled),
+ required_pull_request_reviews: branch_protection[:required_pull_request_reviews].present?
}
new(hash)
diff --git a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb
index 4529c2675ff24..027b2ac422e33 100644
--- a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb
@@ -9,12 +9,17 @@
let(:allow_force_pushes_on_github) { true }
let(:required_conversation_resolution) { false }
let(:required_signatures) { false }
+ let(:required_pull_request_reviews) { false }
+ let(:expected_push_access_level) { Gitlab::Access::MAINTAINER }
+ let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+ let(:expected_allow_force_push) { true }
let(:github_protected_branch) do
Gitlab::GithubImport::Representation::ProtectedBranch.new(
id: branch_name,
allow_force_pushes: allow_force_pushes_on_github,
required_conversation_resolution: required_conversation_resolution,
- required_signatures: required_signatures
+ required_signatures: required_signatures,
+ required_pull_request_reviews: required_pull_request_reviews
)
end
@@ -28,8 +33,8 @@
let(:expected_ruleset) do
{
name: 'protection',
- push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
- merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }],
+ push_access_levels_attributes: [{ access_level: expected_push_access_level }],
+ merge_access_levels_attributes: [{ access_level: expected_merge_access_level }],
allow_force_push: expected_allow_force_push
}
end
@@ -165,7 +170,7 @@
end
end
- context "when branch is not default" do
+ context 'when branch is not default' do
context 'when required_conversation_resolution rule is enabled' do
let(:required_conversation_resolution) { true }
@@ -190,5 +195,107 @@
it_behaves_like 'does not change project attributes'
end
end
+
+ context 'when required_pull_request_reviews rule is enabled on GitHub' do
+ let(:required_pull_request_reviews) { true }
+ let(:expected_push_access_level) { Gitlab::Access::NO_ACCESS }
+ let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+
+ context 'when required_pull_request_reviews rule is disabled on GitHub' do
+ let(:required_pull_request_reviews) { false }
+
+ context 'when branch is default' do
+ before do
+ allow(project).to receive(:default_branch).and_return(branch_name)
+ end
+
+ context 'when default branch protection = Gitlab::Access::PROTECTION_DEV_CAN_PUSH' do
+ before do
+ stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+ end
+
+ let(:expected_push_access_level) { Gitlab::Access::DEVELOPER }
+ let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+
+ context 'when default branch protection = Gitlab::Access::PROTECTION_DEV_CAN_MERGE' do
+ before do
+ stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+ end
+
+ let(:expected_push_access_level) { Gitlab::Access::MAINTAINER }
+ let(:expected_merge_access_level) { Gitlab::Access::DEVELOPER }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+ end
+
+ context 'when branch is protected on GitLab' do
+ let(:protected_branch) do
+ create(
+ :protected_branch,
+ project: project,
+ name: 'protect*',
+ allow_force_push: true
+ )
+ end
+
+ let(:push_access_level) { protected_branch.push_access_levels.first }
+ let(:merge_access_level) { protected_branch.merge_access_levels.first }
+
+ context 'when there is branch protection rule for the role' do
+ context 'when No one can merge' do
+ before do
+ merge_access_level.update_column(:access_level, Gitlab::Access::NO_ACCESS)
+ end
+
+ let(:expected_push_access_level) { push_access_level.access_level }
+ let(:expected_merge_access_level) { Gitlab::Access::NO_ACCESS }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+
+ context 'when Maintainers and Developers can merge' do
+ before do
+ merge_access_level.update_column(:access_level, Gitlab::Access::DEVELOPER)
+ end
+
+ let(:gitlab_push_access_level) { push_access_level.access_level }
+ let(:gitlab_merge_access_level) { merge_access_level.access_level }
+ let(:expected_push_access_level) { gitlab_push_access_level }
+ let(:expected_merge_access_level) { [gitlab_merge_access_level, github_default_merge_access_level].max }
+ let(:github_default_merge_access_level) do
+ Gitlab::GithubImport::Importer::ProtectedBranchImporter::GITHUB_DEFAULT_MERGE_ACCESS_LEVEL
+ end
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+ end
+
+ context 'when there is no branch protection rule for the role' do
+ before do
+ push_access_level.update_column(:user_id, project.owner.id)
+ merge_access_level.update_column(:user_id, project.owner.id)
+ end
+
+ let(:expected_push_access_level) { ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+ let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+ end
+
+ context 'when branch is neither default nor protected on GitLab' do
+ let(:expected_push_access_level) { ProtectedBranch::PushAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+ let(:expected_merge_access_level) { ProtectedBranch::MergeAccessLevel::GITLAB_DEFAULT_ACCESS_LEVEL }
+
+ it_behaves_like 'create branch protection by the strictest ruleset'
+ end
+ end
end
end
diff --git a/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb b/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb
index 4e9208be98541..a0ced456391e8 100644
--- a/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb
@@ -23,11 +23,13 @@
let(:github_protection_rule) do
response = Struct.new(:name, :url, :required_signatures, :enforce_admins, :required_linear_history,
:allow_force_pushes, :allow_deletion, :block_creations, :required_conversation_resolution,
+ :required_pull_request_reviews,
keyword_init: true
)
required_signatures = Struct.new(:url, :enabled, keyword_init: true)
enforce_admins = Struct.new(:url, :enabled, keyword_init: true)
allow_option = Struct.new(:enabled, keyword_init: true)
+ required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true)
response.new(
name: 'main',
url: 'https://example.com/branches/main/protection',
@@ -53,6 +55,10 @@
),
required_conversation_resolution: allow_option.new(
enabled: false
+ ),
+ required_pull_request_reviews: required_pull_request_reviews.new(
+ url: 'https://example.com/branches/main/protection/required_pull_request_reviews',
+ dismissal_restrictions: {}
)
)
end
diff --git a/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb b/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb
index 21fd3b2e44a93..30b29659eee08 100644
--- a/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb
+++ b/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb
@@ -20,6 +20,10 @@
it 'includes the protected branch required_conversation_resolution attribute' do
expect(protected_branch.required_conversation_resolution).to eq true
end
+
+ it 'includes the protected branch required_pull_request_reviews' do
+ expect(protected_branch.required_pull_request_reviews).to eq true
+ end
end
end
@@ -27,9 +31,11 @@
let(:response) do
response = Struct.new(
:url, :allow_force_pushes, :required_conversation_resolution, :required_signatures,
+ :required_pull_request_reviews,
keyword_init: true
)
enabled_setting = Struct.new(:enabled, keyword_init: true)
+ required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true)
response.new(
url: 'https://example.com/branches/main/protection',
allow_force_pushes: enabled_setting.new(
@@ -40,6 +46,10 @@
),
required_signatures: enabled_setting.new(
enabled: true
+ ),
+ required_pull_request_reviews: required_pull_request_reviews.new(
+ url: 'https://example.com/branches/main/protection/required_pull_request_reviews',
+ dismissal_restrictions: {}
)
)
end
@@ -56,7 +66,8 @@
'id' => 'main',
'allow_force_pushes' => true,
'required_conversation_resolution' => true,
- 'required_signatures' => true
+ 'required_signatures' => true,
+ 'required_pull_request_reviews' => true
}
end
--
GitLab