Hi all,
Here are some status updates.
TLDR:
- There is an issue with the migration causing over half of our
issues to be off-by-one issue numbers.
- Further investigation on branch protection shows we cannot get
a suitable setup using .asf.yaml, we will need support.
- Feedback and CI on merge requests originating from forks is
now fixed.
First, the migration issue.
All migrated issues up until #800 have the same issue number in gitlab
and in github, however issue #801 was skipped in the migration, due to
the migration script believing that #801 was in fact the already
migrated #423.
The migration script avoids accidentally migrating the same record
twice, probably in case the script needed to be restarted (which is
did):
https://github.com/Cynical-Optimist/node-gitlab-2-github/blob/master/src/index.ts#L281
The net result, we do not have an issue #801 in github, and every
subsequent following issue in github is numbered "${gitlab_issue} - 1".
This is not the end of the world, but it is quite bad. Most notably, we
need to pay extra attention when merging pull requests which have been
open pre-migration, and check that commit messages and PR comments are
amended to refer to the correct issue number.
On Thu, 2021-01-07 at 22:20 +0900, Tristan Van Berkom wrote:
> Hi all,
[...]
> Todo list
> ---------
>
> * Configuration of protected branches.
>
> This is a little complicated without access to the repository
> configuration UI (I managed to temporarily lock myself out during
> the migration).
>
> The most important part will be to ensure we dont allow any
> non-fastforward merges on master or any release branch, this is
> rather high priority.
I ran some tests today updating the relevant portions of .asf.yaml[0],
and it seems there is no way to properly set this up. We will certainly
need help from the infra team to get this sorted.
I have added the relevant CODEOWNERS file, which does appear to work,
but there is no way to allow self-approval of pull requests on
protected branches[1], so I think that requiring code-owner reviews is
unworkable for us.
Disallowing non-fastforward merges / force pushes on protected branches
is also just not working.
The "Prevent force pushes" section[0] says that specifying a single
branch without options should cause this, but I was unable to reproduce
that behavior, nor do I understand how the .asf.yaml handling code[2]
would cause that to happen, I suspect the wiki is outdated (also, we
need to protect multiple branches, not just master).
I have tried a simpler setup by setting "strict: True" on multiple
branches, without requiring review, like so:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# Testing basic protection of multiple branches,
# this needs to be tested on the master branch.
#
protected_branches:
# Test 1
#
tristan/test-protected-branch-1:
required_status_checks:
# strict means "Require branches to be up to date before merging".
strict: true
# Test 2
#
tristan/test-protected-branch-2:
required_status_checks:
# strict means "Require branches to be up to date before merging".
strict: true
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I received automated emails informing me that:
"GitHub Protected Branches has been enabled on
branch=tristan/test-protected-branch-1"
However, subsequently:
I was able to merge this pull request without rebasing:
https://github.com/apache/buildstream/pull/1437
A second try shows I am still allowed to merge a pull request
without rebasing against the target branch:
https://github.com/apache/buildstream/pull/1439
> * Status checks for CI which is running on pull requests don't seem
> to appear in the pull request UI.
>
> This may be only an issue for pull requests submitted from "forked"
> repositories.
>
> Since we expect most contributions to come from "forked" repos on
> github, it would be good to fix this (and from prior experience, I've
> seen this work properly before on other projects).
>
> E.g.: At the bottom of the page here:
>
> https://github.com/apache/buildstream/pull/1436
>
> We can see some status about the PR, but it is missing the status
> from "workflow actions".
This was fixed by running CI on our repo on the `pull_request` event,
which will be triggered in our repo whenever a pull request is created
even if it is created from a downstream fork.
It is a well known issue that running CI on both "push" events and
"pull_request" events causes CI to run twice, which is wasteful,
however we do want the ability to run CI in advance of creating a pull
request.
While github has yet to provide a solution for this issue, I was able
to find this workaround[3], testing so far shows this is working as
expected.
Cheers,
-Tristan
[0]:
https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features#git.asf.yamlfeatures-BranchProtection
[1]:
https://github.community/t/do-not-require-owner-approval-if-the-pull-request-is-from-an-owner/369
[2]:
https://github.com/apache/infrastructure-puppet/blob/deployment/modules/gitbox/files/asfgit/asfyaml.py#L376
[3]:
https://github.community/t/how-to-trigger-an-action-on-push-or-pull-request-but-not-both/16662/10
