Move codeowner validation checks into Commits::CreateService
What does this MR do?
Iteratively, we've pushed these checks into a number of different locations, however having a unified, single-location for these checks is for more sustainable into the future. By moving the existing "web request" checks into Commits::CreateService, we can remove the existing checks previously added to the file editor, enable checks on the Commits REST API (create/update/delete, cherry-pick, revert), and can remove the :skip_web_ui_code_owner_validations ermergency workaround
Testing
This change has passed automated tests for existing Code Owners scenarios, as well as manual QA testing:
Basic setup steps before testing:
- Create a repo on your development instance
- Enable code owners on a protected branch (
master
is fine) - Create
README.md
,editable.md
, andtest/
directory - Create a
CODEOWNERS
file with entires forREADME.md
andtest/
. Assign one to your account’s @ name, and the other to both you and 2nd user in the app.
File Editor
-
Against file matching CODEOWNERS
-
Create new file — Expected: 🚫 -
Edit existing file — Expected: 🚫
-
-
Against file NOT matching CODEOWNERS
-
Create new file — Expected: ✅ -
Edit existing file — Expected: ✅
-
Web IDE
-
Against file matching CODEOWNERS
-
Create — Expected: 🚫 -
Edit — Expected: 🚫 -
Delete — Expected: 🚫 -
Rename — Expected: 🚫
-
-
Multiple files, one matching CODEOWNERS
-
Create — Expected: 🚫 -
Edit — Expected: 🚫 -
Delete — Expected: 🚫 -
Rename — Expected: 🚫
-
-
Against NOT file matching CODEOWNERS
-
Create — Expected: ✅ -
Edit — Expected: ✅ -
Delete — Expected: ✅ -
Rename — Expected: ✅
-
CLI
- Clone locally
- Change a file on a protected branch
- Push change to gitlab
-
On a protected branch -
Commit contains a file that matches ANY rule in CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches a rule in CODEOWNERS
—✅
-
-
On an unprotected branch -
Commit contains a file that matches ANY rule in CODEOWNERS
—✅ -
Commit does NOT contain a file that matches a rule in CODEOWNERS
—✅
-
API
Existing tests in
spec/requests/api/commits_spec.rb
ee/spec/requests/api/commits_spec.rb
Create commit
post ':id/repository/commits'
-
Create action -
Commit contains a file that matches CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches CODEOWNERS
—✅
-
-
Update action -
Commit contains a file that matches CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches CODEOWNERS
—✅
-
-
Delete action -
Commit contains a file that matches CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches CODEOWNERS
—✅
-
Cherry Pick
post ':id/repository/commits/:sha/cherry_pick'
-
Commit contains a file that matches CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches CODEOWNERS
—✅
Revert
post ':id/repository/commits/:sha/revert’
-
Commit contains a file that matches CODEOWNERS
—🚫 -
Commit does NOT contain a file that matches CODEOWNERS
—✅
Merge Requests
MR contains protected file
-
Approval rule created -
Approval rule displays any/all users listed in the entry
Once approval rules are satisfied
-
User with merge rights can successfully merge regardless of code ownership
Misc
-
blocks file uploads to protected
Addresses #218687 (closed)
Edited by Kerri Miller