tree 48bcd0983fd5241e2131549d411aff95b07e4b50
parent 7a55eccfc0c808d59f90142cc73ce74db939ce0f
author Edwin Kempin <ekempin@google.com> 1600931630 +0200
committer Edwin Kempin <ekempin@google.com> 1601026694 +0200

Make it configurable which files of merge commits need code owner approvals

So far for merge commits we required code owner approval for all files
that differ between the merge commit that is being reviewed and the tip
of the destination branch. This makes sense because:

* some branches may have opted-out of the code owner functionality and
  changes to them do not require code owner approvals, hence when these
  changes are merged into a branch that requires code owner approvals we
  should require code owner approvals for the changed files now

* the code owners may differ between branches and what has been approved
  by the code owners in one branch may not be wanted by the code owners
  in another branch

However this approach also has some issues:

* If the code owners in all branches are the same, the code owners have
  to approve the changes multiple times (once in the branch where they
  have been originally done, and once for every merge into another
  branch).
* If all branches require code owner approvals and the code owners in
  all branches are trusted, it is unneeded to require code owner
  approvals once again on merges.
* By default, for merge commits the change screen shows the diff against
  the auto merge commit (the result of merging the parent commits
  automatically). This means, files that didn't require a conflict
  resolution don't show up (they can only be seen if the user selects
  the first parent as base, instead of the auto merge). It is confusing
  for users, if code owner approvals are required for files that do not
  show up in the change screen. To support this better we would need to
  implement a dedicated UX workflow for this scenario.

To mitigate the issue we now allow to configure a merge commit strategy
that defines which files of a merge commit require code owner approvals.

The supported options are:

* ALL_CHANGED_FILES (current behavior):
  All files which differ between the merge commmit that is being
  reviewed and its first parent commit (which is the HEAD of the
  destination branch) require code owner approvals.

* FILES_WITH_CONFLICT_RESOLUTION:
  Only files which differ between the merge commmit that is being
  reviewed and the auto merge commit (the result of automatically
  merging the 2 parent commits, may contain Git conflict markers)
  require code owner approvals.

Using the FILES_WITH_CONFLICT_RESOLUTION strategy means that files that
have been merged automatically and for which no manual conflict
resolution has been done do not require code owner approval. Hence the
issues above can be avoided.

However using this strategy is only recommended, if all branches require
code owner approvals and if the code owners in all branches are trusted.
If this is not the case, it is recommended to use the ALL_CHANGED_FILES
strategy instead.

Example: If the FILES_WITH_CONFLICT_RESOLUTION strategy is used and
there is a branch that doesn't require code owner approvals (e.g. a user
sandbox branch or an experimental branch) the code owners check can be
bypassed by:

* setting the branch that doesn't require code owner approvals to the
  same commit as the main branch that does require code owner approvals
* making a change in the branch that doesn't require code owner
  approvals
* merging this change back into the main branch that does require code
  owner approvals
* since it's a clean merge, all files are merged automatically and no
  code owner approval is required

Since this can be a security issue, the FILES_WITH_CONFLICT_RESOLUTION
strategy must be configured explicitly and the ALL_CHANGED_FILES is used
by default.

For reference, the find-owners plugin has the same behavior as the
ALL_CHANGED_FILES strategy.

The merge commit strategy can be configured globally or per project.

Which strategy is being used for a project is made available to the
frontend via the GetCodeOwnerProjectConfig REST endpoint, so that it
could customize the UI based on the used strategy.

Change-Id: Ic7709346bcfb85623b62420e9864fbdeb36c45ad
Signed-off-by: Edwin Kempin <ekempin@google.com>