rs-ipfs / welcome

Entry point for the IPFS Rust organization's community and management efforts.
4 stars 0 forks source link

Use of bors(-ng) #6

Closed koivunej closed 4 years ago

koivunej commented 4 years ago

(Created this issue here as of https://github.com/rs-ipfs/rust-ipfs/issues/139).

While rust-ipfs has some flaky tests which prevent enabling it right away, using it in the short term is something I think would be really important. I've been trying out in my own minipb repository.

I remember from grant1 already creating issues on master which bors would had prevented (the case was some three small PRs which I created or at least merged concurrently?). Preventing that alone is reason enough for me to start using it. In more general sense, bors solves the issue of "who will merge this" (reviewer or submitter in case both have push rights) and does the merging right. "Does the merging right" means trying (running the build) the merge before pushing the merge. As far as I understand, this should already cover any fundamental git merging issues that can arise.

bors-ng is the simplest to use solution I've found as it doesn't require running a github app or anything ourselves. Other bors "versions" used by rust-lang projects would require running something somewhere and authorizing it which we don't have resources to do.

I guess authorizing a github app with commit rights is a risk, but github repo setting protecting against force pushs on master should manage that risk quite well. If something bad would happen, cleaning up the master and rolling the HEAD back to an earlier state will be easy.

This does change a bit on the GH PR workflow though:

Experimentation so far:

aphelionz commented 4 years ago

In full support of this. We'll need to make sure the bors commands are pretty visible. You and I can memorize them but if we being in new contributors + reviewers we need a handy spot to keep all the bors documentation so they know what to do.

koivunej commented 4 years ago

Linking the docs here, since these describe the commands https://bors.tech/documentation/ for anyone who hasn't come across them yet.

I'll continue my experiments and update the issue accordingly and when ready, we can start making the changes unless there are any concerns.

aphelionz commented 4 years ago

No concerns, just ping me when the PR is out of draft and I will make the necessary changes to CONTRIBUTING.md and/or other docs. I don't think simply linking the docs is sufficient enough, but I can do the writing work to make it more explicit

koivunej commented 4 years ago

Configuring the reviewers was a bit difficult as it needed to be done through the dashboard which is didn't update for me initially.

koivunej commented 4 years ago

Review approvals work just as well as normal comments.

koivunej commented 4 years ago

Don't really think we have a need case for rollups or squashing right now. I consider this now "ready to go" but we'll still need to find a way to get the CI passing something like 90% or 95% of time.

For reference we played around in https://github.com/koivunej/minipb/pulls

koivunej commented 4 years ago

Bors has been activated and thus this can be closed.

One follow-up I've been thinking that maybe we could just do checking and linting on the push, and use bors to do the actual test and conformance test runs? Well, perhaps this is thinking ahead too much as the try action is guarded resource. Lets just go ahead with full ci suite first on pull_request and then again by bors.