Deepreg: Docs: Branch Merging Guidelines

[NMontanaBrown]

Subject of the issue

During the dev meeting 03/09/20 we discussed that we should have vague guidelines for merging from dev -> main or directly into main. Alternatively, that we should change the PR template such that we discuss why we are requesting a merge into main instead of dev.

We need to reach a consensus, so discuss here

[NMontanaBrown]

@mathpluscode @ebonmati @MattClarkson @YipengHu @RemiDelaunay @YipengHu @QianyeYang @acasamitjana @agrimwood @zacbaum @ZheMin-1992 @s-sd

[YipengHu]

@NMontanaBrown @mathpluscode either of you can provide one or two examples of practice from other repos to discuss on thursday?

[mathpluscode]

With recent usage, I strongly feel like having a develop branch is over-doing the thing, and I had to manually sync between main/develop multiple times for nothing :(

So I propose to remove the develop branch and use main only for the latest version. Stable versions are marked with tags.

What do you think ;) Ofc if you google this issue, there are many discussion like https://softwareengineering.stackexchange.com/questions/312022/the-trend-of-the-develop-branch-going-away.

[mathpluscode]

@zacbaum @s-sd

I just saw your thumbs here, so are we ok for abandoning develop? sorry for this change, but we aim to find the best rules during the practice.

We can take the chance to prepare the guidelines as well?

Based on today's discussion, I propose the following guidelines.

Each PR requires >=1 review, and the person should tag >=2 people to ask for reviews, for instance,

Hi @member1 @member2, would you have time to review this PR?
Please assign yourself if yes. Otherwise, please reply below and tag another person as a suggestion. 

Then we wait until at least one person reviewed the PR.
If the reviewer considers the PR is critical, he/she can suggest having at least two reviewers.

What do you think? @NMontanaBrown @YipengHu

[YipengHu]

i think main-only is a good simpler and modern idea. what is the difference between 1 review with two tagged reviewers and 2 approving reviews?

[mathpluscode]

i think main-only is a good simpler and modern idea. what is the difference between 1 review with two tagged reviewers and 2 approving reviews?

@YipengHu we tag two so that in the best case we have two reviews, but it’s not mandatory. If we tag only one person and that person is not available, this gonna take a lot of time until we get someone for review and get one approval.

We are not a big big project yet, so we should find a sweet point between efficiency and these rules. The code will never be perfect but we will be always improving it.

[s-sd]

@mathpluscode, yes I think main only is a good idea

[zacbaum]

I, personally, would prefer a minimum of two approvals for every PR - but I'm just one vote, so if others don't agree then I am happy with @mathpluscode's suggestion wherein

Each PR requires >=1 review, and the person should tag >=2 people to ask for reviews, for instance,
Then we wait until at least one person reviewed the PR.
If the reviewer considers the PR is critical, he/she can suggest having at least two reviewers.

Though we will have to draw a line at what "critical" is, and seeing as what @NMontanaBrown and I raised yesterday where many do not follow current guidelines, I see this becoming another forgotten process that nobody abides by, and we effectively are no different than where we are now with one approval.

[acasamitjana]

I also think main only is a good idea.
I agree with @zacbaum that at least two reviewers are needed for every PR (tag 3 & self-assign).

[YipengHu]

I see two options:
1- one approval + reviews + sign off key points in reviewing guide (coming up after this discussion). This does allow things like bugfix going in quickly.
2- two approvals. This requires core Dev team responding when requested in 24 hours (at least to say no I don't have time).
As for being critical, I think back-compatibility is a good criterion (will define). Any other things to consider?
I don't have preference, we can try either and adapt if not working.

[zacbaum]

2- two approvals. This requires core Dev team responding when requested in 24 hours (at least to say no I don't have time).

I think 24-48 hours is acceptable but it needs to be made clear that people should not be expected to respond outside of their regular working hours (i.e. 9-5 on weekdays for most).

[NMontanaBrown]

I agree with @acasamitjana and @zacbaum, two approvals and reviews for PRs from different people on core dev team (not including Candi or DeepRegNet account!).

Before we define "times" to get reviews done, we should check in on the team for availability and see what the teams boundaries are. Are we expecting "responses" from team in 24h, even if that includes non-working hours (say, 9-5 Mon-Fri)? What about holidays? This is an OS side project for most, and we have to respect people's time limits for things. If we do not respect their boundaries, we are likely to lose their input on the project.

Also, do we need to do a review "course" to get people on the same page about what appropriate review is and we can be on the same page, to get consistency between reviewers? We seem to have rules for things that people disregard even after it's brought up. How can we get on the same page? How long should people be spending on review?

Not fussed about Main/dev.

Agree back-compatibility essential. How to test?

[mathpluscode]

As we agree on the develop branch, we will remove it once it’s merged. For the PR guidelines, fine for me :) let’s go for two votes and see.

[YipengHu]

I agree with @acasamitjana and @zacbaum, two approvals and reviews for PRs from different people on core dev team (not including Candi or DeepRegNet account!).

Before we define "times" to get reviews done, we should check in on the team for availability and see what the teams boundaries are. Are we expecting "responses" from team in 24h, even if that includes non-working hours (say, 9-5 Mon-Fri)? What about holidays? This is an OS side project for most, and we have to respect people's time limits for things. If we do not respect their boundaries, we are likely to lose their input on the project.

Also, do we need to do a review "course" to get people on the same page about what appropriate review is and we can be on the same page, to get consistency between reviewers? We seem to have rules for things that people disregard even after it's brought up. How can we get on the same page? How long should people be spending on review?

Not fussed about Main/dev.

Agree back-compatibility essential. How to test?

Thought very hard on this.

• Due to different availability, it is unreasonable to get everyone on board before every little back-compatible changes - so two approvals from others seem to be a good starting point? We just need to communicate with the team, but this is exactly what the github for!
• For those do affect back-compatibility, a group meeting or an offline sign-off with majority team is needed. Back compatibility will be reviewed.
• Don;t think time is an issue here. It is more of a suggestion. If you don't respond, the person wanted her/his PR in quickly should ask two others. If S/he communicate first, likely to get quicker response.

We just didn't have those definitions and guidelines - PLEASE pitch in how to define back-compatibility / critical, e.g. what has been disruptive to your work, with concrete examples!

Watch this space - trying to avoid "courses" as much as i can until absolutely necessary :D

[mathpluscode]

@zacbaum @acasamitjana @YipengHu @NMontanaBrown @s-sd I've activated 2 approval requirements for PR into main branch ;)

For the reviewer policy, is there anyone is happy to prepare a draft before the next meeting?

[YipengHu]

@zacbaum @acasamitjana @YipengHu @NMontanaBrown @s-sd I've activated 2 approval requirements for PR into main branch ;)

For the reviewer policy, is there anyone is happy to prepare a draft before the next meeting?

im on it.

[mathpluscode]

Close this as we removed develop and 2 review requests have been activated and working fine.
Reopen this if anyone feels there's a need to define clearer docs ;)