Skip to content

Add "Require code owner approval" setting to protected branches

Kerri Miller requested to merge 10395-require-code-owner-approval-on-pushes into master

What does this MR do?

Initially for API only, this MR adds merge_requests_require_code_owner_approval to ProtectedBranch, an option which, when enabled, will prevent pushing directly to such branches when the contents of the push match against a rule pattern in .gitlab/CODEOWNERS.

This is the initial work to address #10395 (closed)

  • Add merge_requests_require_code_owner_approval to protected_branches table
  • Check the target branch of an MR for protected status in ApprovalWrappedRule
  • Update Gitlab::Checks::BranchCheck#protected_branch_push_checks to prevent pushes to protected branches when that push includes changes to a CODEOWNERS file.
  • Extract CE-specific code for a parallel MR Pipelines will not 💚 until this occurs

Database checklist

When adding migrations:

  • Updated db/schema.rb
  • Added a down method so the migration can be reverted
  • Added the output of the migration(s) to the MR body
  • Added tests for the migration in spec/migrations if necessary (e.g. when migrating data)

When adding or modifying queries to improve performance:

  • Included data that shows the performance improvement, preferably in the form of a benchmark
  • Included the output of EXPLAIN (ANALYZE, BUFFERS) of the relevant queries

When adding foreign keys to existing tables:

  • Included a migration to remove orphaned rows in the source table before adding the foreign key
  • Removed any instances of dependent: ... that may no longer be necessary

When adding tables:

  • Ordered columns based on the Ordering Table Columns guidelines
  • Added foreign keys to any columns pointing to data in other tables
  • Added indexes for fields that are used in statements such as WHERE, ORDER BY, GROUP BY, and JOINs

When removing columns, tables, indexes or other structures:

  • Removed these in a post-deployment migration
  • Made sure the application no longer uses (or ignores) these structures

migration output

== 20190729180447 AddMergeRequestsRequireCodeOwnerApprovalToProtectedBranches: migrating
-- transaction_open?()
   -> 0.0000s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- transaction()
-- add_column(:protected_branches, :merge_requests_require_code_owner_approval, :boolean, {:default=>nil})
   -> 0.0006s
-- change_column_default(:protected_branches, :merge_requests_require_code_owner_approval, false)
   -> 0.0013s
   -> 0.0026s
-- columns(:protected_branches)
   -> 0.0006s
-- transaction_open?()
   -> 0.0000s
-- exec_query("SELECT COUNT(*) AS count FROM \"protected_branches\"")
   -> 0.0004s
-- change_column_null(:protected_branches, :merge_requests_require_code_owner_approval, false)
   -> 0.0004s
-- execute("RESET ALL")
   -> 0.0002s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:protected_branches, [:project_id, :merge_requests_require_code_owner_approval], {:name=>"code_owner_approval_required", :where=>"merge_requests_require_code_owner_approval IS TRUE", :algorithm=>:concurrently})
   -> 0.0007s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- add_index(:protected_branches, [:project_id, :merge_requests_require_code_owner_approval], {:name=>"code_owner_approval_required", :where=>"merge_requests_require_code_owner_approval IS TRUE", :algorithm=>:concurrently})
   -> 0.0018s
-- execute("RESET ALL")
   -> 0.0002s
== 20190729180447 AddMergeRequestsRequireCodeOwnerApprovalToProtectedBranches: migrated (0.0078s)

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Edited by 🤖 GitLab Bot 🤖

Merge request reports