Run CI on pull requests

[nyurik]

This is similar to https://github.com/georust/geo/pull/748 that was just merged

[lnicola]

Can we drop main, since it's not used?

[nyurik]

Can we drop main, since it's not used?

To be honest, I would rather convince the project to rename its primary branch to main -- github now offers a number of features to make this move simpler. This has been a source of a big controversy, with many developers finding master offensive. As always, it is not if "me personally find it non-offensive", but rather -- if something offends someone, why keep using it? There are bazillion (technical term) articles on the topic and why it is being done.

More to the technical merrits -- its a noop here, but gives an easy way to rename branches later, and also gives an easy copy/paste into other georust projects that might have a different default branch.

[lnicola]

Until the main (sic) branch gets renamed, this seems unproductive and quite prone to cause friction, so it's might have been better to not open the subject. I'm new to the project and I don't want to r+ or r- this change based on my personal opinions (it's the decision of the main authors), so I'll recuse myself from the discussion.

As for my opinion, I tend to agree with the author of this, who's actually in the demographics targeted by the those arguing for banning that specific word.

[michaelkirk]

I've gone ahead and renamed the default branch to main.

Locally you can run:

git branch -m master main
git fetch origin
git branch -u origin/main main
git remote set-head origin -a
git remote prune origin

[nyurik]

Thank you @michaelkirk !! I have added another step git remote prune origin -- helps to keep remotes stored locally clean.

[michaelkirk]

Before I merge, is there a reason I should not reduce this permission to "Read repository contents"?

[lnicola]

That's useful in some cases -- a workflow might file a new issue --, but not for us.

[michaelkirk]

bors r+

[bors[bot]]

Build succeeded:

• ci result

[michaelkirk]

For the record I set these permissions:

(and also applied them to geo)

Overall the end result seems slightly more convenient for internal contributors (because we'll automatically build PR's from their forks), while being more-or-less the same for outside contributions.

[lnicola]

I'd leave the first one on the second option. It's pretty annoying when things are too locked down. I was recently asked to confirm a fix for an issue a week after the issue got locked for non-collaborators. That feels needlessly hostile.

Besides that, it's safer to allow CI to run without approval than to give new contributors commit rights because they can't access the secrets from their own forks.

[michaelkirk]

they can't access the secrets from their own forks.

Can you elaborate on this?

My understanding is that our CI (including any secrets it has) runs their code. Otherwise how could you build a PR from a fork if CI depended on using those secrets?

[lnicola]

You can't, IIUC. From the settings page:

Secrets are not passed to workflows that are triggered by a pull request from a fork.

[nyurik]

correct, any CI originating from the code outside of this repo cannot access any secrets. The biggest limitation this causes is that if one wants to post a comment to the PR with the results, they cannot do it directly. (we often want to create an automated build summary comment, showing if the PR passes, and what the result would look like). The workaround is to have a CI job that listens to the completion of the build -- in other words, we could have this workflow:

• a PR from a fork triggers "build" job
• the "build" job cannot access any secrets, but it can create build artifacts, i.e. some message like "everything is ok, result is XYZ"
• once the "build" job finishes, it triggers the "monitor" job that runs as part of the main repo, with access to all tokens
• the "monitor" job looks at the finished "build" job's artifacts, and posts them as comments

[michaelkirk]

Thanks to you both for explaining this! I'm happy to see that things seem more locked down than I feared.