Skip to content

Conversation

@pietroalbini
Copy link
Member

While for most use cases running the CI builds on the same repository is
fine, if a lot of people have write access to the repository there might
be security concerns. To placate those this adds a new feature to Homu
allowing the actual CI builds to be executed in a fork with tighter
permissions.

The implementation relies on how GitHub stores forks in their backends:
all the forks of a repository plus the main one are actually stored in
the same underlying git bare repo. This means that if we push the merge
commit to the fork we can instantly fast-forward the main repo's master
branch to that commit, without the need to reupload things. This
simplifies the Homu logic a lot, and because of that the changes needed
to implement the feature were minimal.

A downside of that is the feature requires the fork to be an actual
GitHub fork. Unfortunately GitHub doesn't allow the main repo to be in
the same account/org as a fork of it, so we'll have to store the rustc
fork in another org. Thanks to the team repo managing access to it
shouldn't be a problem.

Another downside is the commit history on the main repository won't show
the green checkmarks, as the builds happened on the fork. This downside
is mitigated by the fact that Homu will keep the history on the fork in
sync with the main repo, allowing users to see the checkmarks on the
fork instead. Thanks to this the UX loss will be minimal.

This PR makes three different changes to homu to support the feature:

  • Homu will consider the fork an alias of the main repository when it
    receives a webhook, allowing it to process CI results from the fork.

  • When the feature is enabled, Homu will push the merge commit in the
    fork's auto and try branches instead of the main repository.

  • Once the build is green, in addition to fast-forwarding the main
    repository's master branch to the merge commit Homu will also force
    push the merge commit to the fork's master branch. This will allow
    users to follow the commit history on the fork as well.

While for most use cases running the CI builds on the same repository is
fine, if a lot of people have write access to the repository there might
be security concerns. To placate those this adds a new feature to Homu
allowing the actual CI builds to be executed in a fork with tighter
permissions.

The implementation relies on how GitHub stores forks in their backends:
all the forks of a repository plus the main one are actually stored in
the same underlying git bare repo. This means that if we push the merge
commit to the fork we can instantly fast-forward the main repo's master
branch to that commit, without the need to reupload things. This
simplifies the Homu logic a lot, and because of that the changes needed
to implement the feature were minimal.

A downside of that is the feature requires the fork to be an actual
GitHub fork. Unfortunately GitHub doesn't allow the main repo to be in
the same account/org as a fork of it, so we'll have to store the rustc
fork in another org. Thanks to the team repo managing access to it
shouldn't be a problem.

Another downside is the commit history on the main repository won't show
the green checkmarks, as the builds happened on the fork. This downside
is mitigated by the fact that Homu will keep the history on the fork in
sync with the main repo, allowing users to see the checkmarks on the
fork instead. Thanks to this the UX loss will be minimal.

This PR makes three different changes to homu to support the feature:

- Homu will consider the fork an alias of the main repository when it
  receives a webhook, allowing it to process CI results from the fork.

- When the feature is enabled, Homu will push the merge commit in the
  fork's auto and try branches instead of the main repository.

- Once the build is green, in addition to fast-forwarding the main
  repository's master branch to the merge commit Homu will also force
  push the merge commit to the fork's master branch. This will allow
  users to follow the commit history on the fork as well.
@pietroalbini
Copy link
Member Author

Anyone from @rust-lang/infra willing to review this?

Copy link
Member

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after fixing comments

@pietroalbini pietroalbini merged commit 1e0041c into master Oct 16, 2019
@pietroalbini
Copy link
Member Author

Thanks for the review @kennytm!

@pietroalbini pietroalbini deleted the test-on-fork branch October 16, 2019 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants