Topics

Merge other developers PRs?


danielb987
 

Since Bob Jacobsen focuses on his signalling project which he deserves to do, it seems that there is no one merging other developers PRs at the moment.

I'm afraid that the result is a big buildup of unmerged PRs waiting to be merged, with all the problems that may bring with it, for example collissions between PRs.

I'm not a maintainer so I can't merge PRs, but I think we should deal with it before we get too many unmerged PRs.

I suggest that authors that have PRs ready to be merged writes a note in the PR: "This PR is ready to be merged", in order to help maintainers to see which PRs is ready to merge.

I also suggest that the maintainers step up and review PRs marked as ready to be merged. If you want my review on PRs, feel free to ask, either here in this thread or in the PR itself. I'm @danielb987 on Github.

Daniel


Dave Heap
 

Daniel,

On 24 Jul 2020, at 2:29 PM, danielb987 <db123@...> wrote:

Since Bob Jacobsen focuses on his signalling project which he deserves to do, it seems that there is no one merging other developers PRs at the moment.

I'm afraid that the result is a big buildup of unmerged PRs waiting to be merged, with all the problems that may bring with it, for example collissions between PRs.

I'm not a maintainer so I can't merge PRs, but I think we should deal with it before we get too many unmerged PRs.
I've got quite a bit of ESU-specific decoder XML to merge but I've held off even raising a PR pending the possible changes and hence not being sure which branch/tag to start with and which branch to PR against.

If it can be made clear what we are doing for 4.21.1 that would be very helpful. Are we still going to use "master"?

Once this is made clear, I'll not only proceed to raise my PR(s) but also look at merging existing PRs with milestone 4.21.1, pending appropriate review/potential impact/discussion.

Given the current climate, I'm not happy to do anything until it's made clear (preferably by Bob) how we are proceeding at this stage.

Dave.


Ken Cameron
 

Once upon a time we tried things like having WIP in the name for the PR
until we thought it read to merge. You removed the WIP when it was ready. We
also used the comments and flat out said "ready to merge" too.

The way I read what Bob J had said left me with the feeling he'd hold off on
some of the work in the hopes we could discuss and get more convergence on
things. IE, he was taking a short holiday to let us figure out a few things.
While the discussions have been good, I'm not sure where we stand on any
convergence of ideas.

I think what I have been reading is that it takes more than one set of eyes
on a PR to make it ready to merge. But for issues beyond that, I'm unsure of
what we have agreed with.

-Ken Cameron, Member JMRI Dev Team
www.jmri.org
www.fingerlakeslivesteamers.org
www.cnymod.org
www.syracusemodelrr.org


Steve Todd
 

Could we create and use "labels" for communicating amongst the group? 

"Needs Review", "Ready to Merge", etc.? 

Seems like something other teams would have come up with a standard for.

--SteveT


Paul Bender
 

On Jul 24, 2020, at 10:11 AM, Ken Cameron <@KenC57> wrote:

Once upon a time we tried things like having WIP in the name for the PR
until we thought it read to merge. You removed the WIP when it was ready.
We still use the WIP in the title to indicate this.

I think what I have been reading is that it takes more than one set of eyes
on a PR to make it ready to merge. But for issues beyond that, I'm unsure of
what we have agreed with.
I don’t know that we have agreed to much else yet.

It boils down to now we need to decide:
1) What are we going to set as ground rules for PR approvals?
2) Are we going to open any additional communication channels?

I could write something up tonight as a starting point for circulation if the group would like.

I think the discussion of how we release ( semantic versioning or the previous method ) should be a separate discussion from the how do we work together discussion.

Paul


danielb987
 

2020-07-24 19:39 skrev Paul Bender:
I could write something up tonight as a starting point for circulation
if the group would like.
Yes, sounds good.

I think the discussion of how we release ( semantic versioning or the
previous method ) should be a separate discussion from the how do we
work together discussion.
I agree.

Daniel


Dave Heap
 

All this discussion about reviews, consensus etc. is good and fine.

However, we now have 111 merged and at least 4 open PRs with milestone 4.21.1 merged into master.

Do we just continue with merging 4.21.1 milestones into master until such time as we have resolved our discussions and then pick up the new process by varying https://github.com/JMRI/JMRI/issues/8831 so that we lock and merge master back into dev-minor for the release of 4.21.1, thence proceeding with the new process?

That's what I'd like to know before merging any more PRs into master (although some other maintainers have already done so in the past couple of days).

Dave

On 24 Jul 2020, at 3:19 PM, Dave Heap via groups.io <dgheap@...> wrote:

If it can be made clear what we are doing for 4.21.1 that would be very helpful. Are we still going to use "master"?

Once this is made clear, I'll not only proceed to raise my PR(s) but also look at merging existing PRs with milestone 4.21.1, pending appropriate review/potential impact/discussion.

Given the current climate, I'm not happy to do anything until it's made clear (preferably by Bob) how we are proceeding at this stage.


Paul Bender
 

Dave,


On Jul 25, 2020, at 12:00 AM, Dave Heap <dgheap@...> wrote:
All this discussion about reviews, consensus etc. is good and fine.

However, we now have 111 merged and at least 4 open PRs with milestone 4.21.1 merged into master.

Do we just continue with merging 4.21.1 milestones into master until such time as we have resolved our discussions and then pick up the new process by varying https://github.com/JMRI/JMRI/issues/8831 so that we lock and merge master back into dev-minor for the release of 4.21.1, thence proceeding with the new process?

That's what I'd like to know before merging any more PRs into master (although some other maintainers have already done so in the past couple of days).

If I were still in the maintainers group, I would be merging the Open PRs that have passed the required status checks.

Paul


Dave Heap
 

Paul,

On 25 Jul 2020, at 10:18 PM, Paul Bender <paul.bender@...> wrote:

If I were still in the maintainers group, I would be merging the Open PRs that have passed the required status checks.
Thanks for that opinion. I'm thinking the same way.

Dave


Bob Jacobsen
 

We’re starting to see overlaps in the 41 outstanding PRs.

Although other aspects are not clear, it seems we do have a consensus that we can merge PRs with one or more positive non-author reviews, no outstanding comments, and required CI complete.

Unless somebody objects, late this evening I’ll start merging PRs that meet those criteria, from lowest numbered up.

Let me know if you’re prefer I not do this.

(And I’d appreciate somebody taking a look at #8869, #8879, #8883, #8887, and #8892)

Bob

Bob Jacobsen
@BobJacobsen


Dave Heap
 

I support that.

Dave

On 5 Aug 2020, at 6:53 AM, Bob Jacobsen <@BobJacobsen> wrote:

We’re starting to see overlaps in the 41 outstanding PRs.

Although other aspects are not clear, it seems we do have a consensus that we can merge PRs with one or more positive non-author reviews, no outstanding comments, and required CI complete.

Unless somebody objects, late this evening I’ll start merging PRs that meet those criteria, from lowest numbered up.


danielb987
 

Mee to.

Daniel

2020-08-05 01:29 skrev Dave Heap:

I support that.
Dave

On 5 Aug 2020, at 6:53 AM, Bob Jacobsen <@BobJacobsen> wrote:
We’re starting to see overlaps in the 41 outstanding PRs.
Although other aspects are not clear, it seems we do have a consensus that we can merge PRs with one or more positive non-author reviews, no outstanding comments, and required CI complete.
Unless somebody objects, late this evening I’ll start merging PRs that meet those criteria, from lowest numbered up.