Contributing - length a PR is open before PR can be merged

[rfergason]

In our meeting notes from 9 Jan 2020, we have an open question about how long a PR can stay open before they get accepted. The Planetary Science TSC documents that we are borrowing state that PRs must be open for 24 hours, where as I remember us informally agreeing that something closer to 48 hours might work better. What is our final decision? (for the record, I don't have a strong opinion. I just prefer loose ends to be tied up nicely).

If we have a different length of time that a PR can remain open, could simply state that difference on the front page without re-generating the entire Planetary Science TSC document with a single change?

[rbeyer]

I can't remember if we came to a conclusion. Seems like that would have made it to the notes if we had. Honestly, I don't think that 48 hours is going to make a difference, and I think we should stick with 24. I appreciate that an extended period for review is meant to allow people more time to review something before a change is made. However, this assumes that 'changes' are hard. This is true in almost all other models of group work that people are used to.

However, in my opinion, the breakthrough of this git-based collaborative model is that changes are (and should be) easy. If we keep it at 24 hours, here's what might happen:

• T = 0: PR filed by Alice
• T = 2h: PR approved by Bob
• T = 10h: comments lodged by Clarisse, resolved by Alice
• T = 12h: PR approved by Clarisse
• T = 24h: PR merged by Alice, since it has now been open for 24h and has two approvals
• T = 36h: Darin notices something about the PR that should be changed!

Under a 48h period, Darin would be able to suggest changes to Alice on her PR, and his concern could get addressed in that merge. However, under a 24h period, Darin's concern would need to be filed as a new PR. Yes, this means that there is the need for an additional round of two approvals and 24h from that PR, but this process should be as 'inexpensive' as any other.

Additionally in that circumstance, does the PR now need two more approvals after the Darin-suggested changes that Alice made, since Clarisse and Bob's approvals were for an earlier version of the PR? Yes, it probably does, so even there, you're not really saving anything.

[jessemapel]

I think this is something I forgot to write down in the notes. I remember the discussion being about how often people engage with github. A 24 hour window would mean everyone needs to check github about twice a day. A 48 hour window would be about once every day.

For me, there is not a huge difference but for people who don't work on github everyday, is there a preference?

[rbeyer]

I disagree, it doesn't mean "everyone" needs to check GitHub at a certain frequency. Additionally, GitHub has a robust notification mechanism that people can choose to engage with or not.

If the concern is that changes to this repo need more validation from the TC then we should really be talking about changing the number of approvals needed, not the time window, as that would more directly serve that concern. Right now, with 24 h, it still means that one person thought it was a good idea (the PR author) and two other thinking human beings approved of that idea.

[AndrewAnnex]

48 hours is reasonable, I believe

[AndrewAnnex]

oops

[rfergason]

I still feel that 48 hours is reasonable.

@rbeyer point is also well made: "If the concern is that changes to this repo need more validation from the TC then we should really be talking about changing the number of approvals needed, not the time window, as that would more directly serve that concern."

[rbeyer]

This is maybe something we need to talk out at a TC meeting.

However, I'll add another perspective: the USGS developers. In a typical, low-to-moderate activity project, having a longer waiting time maybe isn't an issue, but for the ISIS codebase, USGS developers are doing all kinds of work, daily. Are you going to be impeded by the fact that PRs just have to sit around for two days, even if they have 100 approvals? Is this going to cause your developers unnecessary churn because they can't just rebase on master, and they really may need to be basing their work on a PR branches that may or may not land?

Making this 'wait time' longer, will most likely encourage large, complex PRs, and discourage smaller, atomic PRs, which I think is unwise.

[jessemapel]

I prefer 48 hours on the TC repo. I don't expect a huge velocity of changes on this repo and I think having PRs open for 48 hours will ensure that everyone has a chance to see them before they are merged. I don't expect people to be working on the TC full time, so a slower iteration cycle that allows more people to be involved would be beneficial.

On the ISIS repo, I think either 2-approvals with a minimum of 24 hours is a good standard. The ISIS repo does have a much higher velocity of changes and there are people working full time on the project. If it takes too long to iterate on work in the repo, then the devs at the ASC will probably need to do more work in feature branches so that we can share code and avoid merge conflicts at a reasonable pace.

Looking at the recent PRs on ISIS. The smaller PRs that didn't have much review feedback were open around 30 hours. The PRs that had more review feedback were open for longer than that based on how long it took to iterate.

Looking at the recent PRs on this repo. Over half of the PRs were just meeting notes and such. The more substantial PRs remained open for a good while as discussion commenced.

[rbeyer]

Okay, I've made my argument, but no one else seems to agree. I will say it once more, and then rest my case:

If you want more involvement from people, require more approvals (3 approvals, some percentage-round-down of the number of TC members or something), not more time. Increasing the time does not guarantee more involvement, but increasing the required number of approvals does.

[scsides]

I'm with Ross on this one. Think about the different cases: 1) Meeting notes, probably doesn't make a big difference either way, but more approvals will get more people to read them ++, 2) Policy/process/rule changes, absolutely want more approvals even if it takes a few days. This is not where we want to move fast, more approvals = more buy-in ++. But, there still needs to be a minimum approval count with some longer time span.

From a previous discussion, it doesn't seem reasonable to apply the rules for a governing group to the software repo. The two processes are not the same. Rules for the software reop should be brought up on the repo site (issue?, RFC?), so all devs can see it and weight in. Even most Astro devs don't read the TC discussions often, and others may not even know this group exists.

[michaelaye]

somehow i get the feeling that the ISIS and ISIS_TC repos are conflated here? Otherwise why does @rbeyer refer to USGS developers when this issue is really about changes to the ISIS_TC, IIUC?
My opinion is, as ISIS_TC is about steering policy, changes actually should have both: more time AND more required approvals. Why? Because thinking policy is hard (harder for me at least than coding) and I think it's just fine to let the thoughts linger on it for 2 days. And the increased engagement really comes, as previously said, by asking for more approvals.

Having said that, in the spirit of "changes should be easy" (which I 100% agree and support), we could also just run with any of the suggested solutions for a while and if we don't like it, we change it?

[rbeyer]

somehow i get the feeling that the ISIS and ISIS_TC repos are conflated here? Otherwise why does @rbeyer refer to USGS developers when this issue is really about changes to the ISIS_TC, IIUC?

Yes, I wasn't being specific, and was conflating, my apologies.

Having two separate policies is fine, but I still think the 'real' lever arm is number-of-approvals, not necessarily time-to-wait, especially since if we discover a problem with a previous change, another PR can always be lodged. Always.

Having said that, in the spirit of "changes should be easy" (which I 100% agree and support), we could also just run with any of the suggested solutions for a while and if we don't like it, we change it?

We already are: 2 approvals, 24 h. Has this been a problem? Are we trying to solve an issue that doesn't need fixing?

[jlaura]

Without a PR or additional conversation by the next meeting (7.14.2020), we are considering closing this issue. Please feel free to reopen at a later date if this is still a pertinent issue.

[rfergason]

Based on the above comments, we have not reached consensus.

The ISIS_TC has adopted the Contributing Document from the Planetary TSC. So to adopt any of the suggestions above, we either must write and support our own Contributing document or make (and discuss) these changes at the Planetary TSC level. Since we are a subsidiary of the Planetary TSC, I think it makes sense to remain consistent with the Planetary TSC documentation and am not in favor of having our own Contributing document.

I am recommending that we make no changes to the Contributing document (https://github.com/USGS-Astrogeology/ISIS_TC/blob/master/Contributing.md). Thus, to make changes to this repo, the PR must remain open for 24 hours and must have at least 2 approvals. If I see two thumbs up on this suggestion, I will close this issue.

[rfergason]

Since we have two thumps up (2 approvals) and it has been longer than 24 hours, I will close this issue.

The accepted resolution is that we make no changes to the Contributing document (https://github.com/USGS-Astrogeology/ISIS_TC/blob/master/Contributing.md). Thus, to make changes to this repo, the PR must remain open for 24 hours and must have at least 2 approvals.

Please open a new issue or PR to modify this policy.

[jessemapel]

The remaining question is if we want to escalate this to the TSC? I can add it to the agenda for next meeting.