Skip to content

Fix MR approval by reporters

What does this MR do?

Fixes #287795 (closed).

We allowed reporters to approve MRs in !40491 (diffs).

But at the moment this feature requires a protected branch with Require approval from code owners enabled:

Screenshot_2021-06-03_at_13.02.18

The original requirement was stated as:

Users that have no access to modify the code (Reporters) ought to still be able to Approve MRs and Deploy to Protected Environments when they are designated as approvers or deployers

And here is the implementation discussion: #225485 (comment 402492643)

None of them mention requiring the protected branch with Require approval from code owners enabled.

And these features seem to be completely orthogonal.

From @sean_carroll's comment:

Re-reading the code, it seems to be requiring a CODEOWNERS entry, which is probably incorrect.

Also, there are no tests verifying this Require approval from code owners behavior, which also suggests it was added by mistake.

So, this MR just removes Require approval from code owners requirement.

I'll assign it to Product for approval of this change as well, but to speed up things, I'll submit it for backend review earlier.

p.s. I'm actually not sure if should have a requirement for protected branch at all, you can create an approval rule for all branches without creating any protected branch. And in such cases, reporter approvals also won't work. But it's a much smaller bug, and I think we should go with this small fix, and maybe discuss removing the protected branch requirement or adding a special case for all branches later.

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Vladimir Shushlin

Merge request reports

Loading