adding protected branch?

[Dan Kortschak]

Github has added features to allow a branch to be protected - requiring
a review before merge. How do people feel about turning this on for the
gonum repositories.

The settings I would suggest if we do are to protect master, requiring
a review, dismissing stale review approvals and requiring branches be
up to date (all of these things are things that I have found can cause
problems). We would leave off the apply to administrators, but that
would be only used in urgent "fixes build" situations.

Thoughts?

Dan

[Brendan Tracey]

What does “dismissing state review approvals” mean?

Is it easy to turn on and off? We could also enable the protections against administrators, if they’re easy to dismiss in case a particular commit needs it. Our test suite has gotten much better, and so we haven’t had very many urgent fixes recently.

> --
> You received this message because you are subscribed to the Google Groups "gonum-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to gonum-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Dan Kortschak]

On Mon, 2017-06-26 at 21:00 -0600, Brendan Tracey wrote:
> What does “dismissing state review approvals” mean?
>
> Is it easy to turn on and off? We could also enable the protections
> against administrators, if they’re easy to dismiss in case a
> particular commit needs it. Our test suite has gotten much better,
> and so we haven’t had very many urgent fixes recently.

"Stale" - if someone has pushed commits to a PR after approval, it will
require re-approval.
It's reasonably easy to turn on and off, but when there is no
protection enforced on admins, there is still an extra step that is
required for an admin to circumvent the protection; you must click a
checkbox asking to allow it and then do the normal merge/rebase/squash.
The buttons are all red until approval has been obtained. I think
that's probably enough.

[Dan Kortschak]

So is this a thing to do?

[Brendan Tracey]

Yes.

The only other concern I have is the definition of "branches being up to date". Is there somewhere to read exactly what this means? Can several parallel (nonoverlapping) PRs be merged without rebasing on to master?

Brendan

[Dan Kortschak]

It says "This ensures the branch has been tested with the latest code
on master" for that. This is part of the reason I want it - the other
day I had two non-conflicting PRs that touched different things that
interacted (the IsZero and Cap PRs - Cap failed the PR tests and not
the push tests).

Note that the protections prevent rebasing locally and then pushing
directly to master (my old favourite technique), but the rebase button
does essentially the same thing.

[Dan Kortschak]

This is done now. If it is onerous to people (though please give it
time) we can revert this.

[Dan Kortschak]

OK. I've just seen what this does and I don't like how they are doing
it.

It *does* require that the PR be a child of master, rather than just do
a test of the results of the travis pr test result (in hind sight this
is obvious since that information is not integrated by github
semantically more strongly than as a test failure). Worse, the "update"
button does a merge of the master (it should at least offer a rebase
option - I intensely dislike spaghetti histories).

I have turned that option off on the basis that it will be caught by
the travis pr tests.

On Thu, 2017-06-29 at 16:58 -0700, Brendan Tracey wrote: