Pull Requests Sitting Around

[DanVanAtta]

We have a long list of pull requests sitting around. They're small and focused as requested, sometimes even trivial like #33. So perhaps size or complexity of a pull request is not the reason they are not being merged.

So what is going on? Why are so many pull requests sitting around? What are the various things we can do to address this problem?

edit skipping the details of why this is a problem. I'd like to keep this discussion as matter of fact as possible. Though without further elaboration, I'll state this is a very significant problem in my point of view.

[gaborbernat]

I think at the moment this mostly boils down to @ron-murhammer and @veqryn not having much time to do actual reviews, and merges. Don't see any real solutions for this, other than patience or taking a leap of faith and granting commit rights to official repository for you. I thought having easy to review commits would fasten up this process, however I see not. Must understand they have other jobs too, although true it can be demotivating to contribute with slow pull merges.

[DanVanAtta]

edit Fixed up for conciseness/clarity

This boiling down to two or maybe three people not having enough time is not very sustainable. We do need more contributors, but I don't think we'll get any more anytime soon. Partly because of the problems we have with so few contributors. It is a bit of a death spiral.

Idea one: decentralize the reviews

Have a pool of contributors that can merge. Once a pull request has hit N reviews, then it is merged. N for us with so few people would be 1. N=2 is much more ideal and would be more appropriate IMO around 7+ people. I like this approach since then everyone can participate in the reviews, enables a maximized shared work load.

Idea two: second branch with the decentralized reviews (gitflow workflow, develop decentralized, master is centralized)

I think this is a bad idea. Having any two people with this commitment (for years/decades) is not a good idea. Regardless...

We set up a second repo that would be the second branch (develop) as in the gitflow workflow. The pool of contributors instead have admin rights for that repo instead. @veqryn and @ron-murhammer then control the release branches created in the main repo (for stabilization and bug-fix merges, part of the gitflow workflow) and then merging those release branches into master. Finally they would create release artifacts.

[DanVanAtta]

Prior comment has been updated quite a bit, so re-read if you're coming from email.. Another thought about decentralized reviews, instead of N depending on team size, it could alternatively depend on complexity. Simple reviews have n=1, more complicated stuff then n = 2 or 3 even.

[veqryn]

I do plan on giving more people (like you both) full access, but just not quite yet. I prefer to see the quality of people's work over some time period in order to judge if they should be committing/merging directly to master or not. And perhaps more importantly, I like to see what direction they are taking, what their focus is, etc, to see if they are a good fit. We've had people before who's vision for the project did not mesh, and it would have been bad if I gave them commit access.

I do hope to look at more of your patches soon, but I also have a very demanding job, and I hope you can be somewhat understanding if I do not reply for stretches of 3-4 days, etc, because of the rest of my life.

[DanVanAtta]

@veqryn , thanks for hopping in on the discussion so quickly. Part of my concern is that even if we get a few more admins, we may still be having this same discussion, but instead of me raising the issue it'll be someone else, and I'll be the one mentioning other life priorities ;)

Regardless, I think the bottom line is patience. We should still encourage others to review pull requests just so we can get more eyes on the changes.

Is there anything to be done in the short term to make PR's easier to review or less work? Or go ahead and keep letting them stack up (as nice little bite sized chunks of :+1: )?

[DanVanAtta]

While we're on the topic, merging these two pull requests would be really helpful:

43 - add unit test libs (hamcrest/mockito)

58 - fix gradle build and add travis CI

Pretty much all other branches are based on top of those two, so having them be merged would make things a good bit easier for me. (#43 is very quick since it is just new libraries which we discussed previously, #58 is quick if you take leap of faith on it, or could be more involved in which case look at just #43 for now)

[veqryn]

One thing I like to encourage people who are interested in commit access, to do, are the following:

1. Make a map. It doesn't have to be good, but it does have to work. If you can make a basic map with 2 sides, it means you will understand a lot of about the map making process, how the rules work as applied to maps, etc. (if you haven't already)
2. Play a lot of triplea with people, both in the online lobby, and play-by-forum (over at http://www.axisandallies.org/forums/) (if you haven't already). This will teach you about how the methods of play work from the end user's perspective.
3. Play several different maps with different rules sets (if you haven't already). My recommendations is that everyone should be very familiar with the rules sets and maps: World War II v2 Revised World War II v3 1941 (aka: AA50) World War II Global 1940 and some non-standard custom game, such as Napoleonic Empires or 270bc This will show the variety of rules and their customization, that we support.

I always feel a lot better when the people who are contributing are also end users, who play/participate in the community (the online lobby, the play-by-forum forum, and map making).

This isn't a requirement of course. But I did encourage redrum to do the above and I think it greatly helped his understanding of the engine, and the quality of the AI he was/is building.

[ron-murhammer]

I pretty much second everything @veqryn said. Playing a bunch of different maps against both players and the AI helps to get a strong understanding of all the various rule sets. I've finally just begun learning Global 1940 the last few months.

I was also in a very similar position a little over a year ago when I first started contributing as you are in now @DanVanAtta and sometimes got frustrated on response time. Back then it was essentially only @veqryn doing any development work. My availability the last few weeks has been terrible as I've been traveling for work but hope that changes soon.

I would really like to tag a release so that we have a sort of 'stable' point before we start merging lots of changes. Then when @veqryn and I get some time we'll actually do all the builds and release the next version (1.8.0.6) from it. This way we can start merging changes without as much concern.

The alternative as you mention is using gitflow and allowing more people access to a dev branch. This makes things more complex but is a little safer since master stays untouched until @veqryn or I merge changes from dev into master. I think I'd prefer keeping it simple and just merge changes to master but if you think gitflow would make things better I'm open to it.

[DanVanAtta]

I only disagree a bit about the map @veqryn . We have too many maps, not enough players. Despite that I have started toying around with a Risk map. There is a lot of other low hanging fruit first though. The game engine regrettably has a lot of reasons why a risk map is not possible. Some of the refactor changes you've seen have been a way to help make that map possible. Not only that there are a lot of low hanging fruit in terms of easy and useful features to add.

I was playing the game on my new laptop and was hating life because the arrow keys couldn't scroll fast enough. Hence where that feature came from.

I also don't know if our times on lobby have lined up very well. Over the last six years I've been an active player on/off, a few hundred online lobby games. Never had too much interest in playing the AI. Did so a few times to learn maps or check out the AI. Back then the AI was not good, so that was a while ago too.

For more bona-fidas, ask champ, I've played him a whole lot, some interesting maps there. W.r.t: I never liked global so much, but I played it a half dozen times and know the scramble and politic rules. I'm a 'big world v3' expert (non-v3 as well, but wouldn't call myself an expert there anymore), with lots of prior experience on wwII 1941, 1942, NWO (lebowski and smalls - I played in RailRoads NWO tourney back in the day), cold war, cold war 1965. Domination - no mans land is a lot of fun too, but perhaps needs some more balance issues.

I can see the concern of the occasional developer coming through and wanting to do something. They do something, but it's not very good and causes trouble. The plus side, git helps with that. Github etc is a set of tooling that makes that process easier, and for them, submitting a pull request and going on their way will be plenty. Assuming it's reasonable and is not rejected, that is a win-win.

Myself, @gaborbernat and @djensen47 are certainly the obvious candidates for admin. With honor system that reviews are needed for code changes, it would be hard to go to far in the wrong direction. So again, since I don't necessarily disagree, bottom line does seem like some patience then.

[veqryn]

I don't mean "make a map" in the sense that I actually want a playable map that will go into the repo.

I mean "make a map" in the sense of actually going through the process of making a map and getting it to work without bugs, all just to understand the process and how the rules work within the map's xml document, etc.
Use the basic "default" rules (either Revised [v2] or AA50 [v3]), and keep it extremely simple that way there aren't any/many bugs.

Please don't publish the map, unless it is amazing, because you are right, we have too many.

You are doing it more as a fun exercise that will teach you a surprising amount about the internals of triplea.

[DanVanAtta]

@ron-murhammer

Let's go with the one-branch with feature branch model as discussed in the other thread and not switch to gitflow. It won't help. What helps is having a pool of people to do reviews. Sounds like we don't have that pool of people anyways.

@ron-murhammer, @veqryn , If you're waiting for a tag, please go ahead and add it and push it:

$ git fetch origin
$ git tag -a v1.8.0.6  -m '1.8.0.6'
$ git push origin --tags

But note, you can add it retroactively to any commit. So you could at any point in the time in the future in theory, could add a tag to the current master by specifying its SHA: $ git tag -a v1.8.0.6 -m '1.8.0.6' 5e934bad4029b4a33769230d48afcf997a138391 (https://git-scm.com/book/en/v1/Git-Basics-Tagging)

[veqryn]

unrelated but fyi, the next release is 1.8.0.7 odd numbers = releases even numbers = developer builds

[djensen47]

I agree with no git-flow, ever. When you have a lot of feature branches, it can become a nightmare to manage.

Feature branch and a master branch is good with me. In a sense, "git-flow lite."

[DanVanAtta]

Thanks for all the commentary and useful discussion. Let's wrap this conversation up and also the work flow one (#60) as well. Let's focus our efforts on pull requests and un-resolved issues (like: #60)

I think there is overall concensus. @veqryn , the exercise you mention is a good one. I've gone through parts of it. I'd note though that a typical open source contribution usually has three phases:

• lurking in the community. Reading what happens, getting a feel for the pulse of what is going on. For this I started reading the source forge stuff to get a history of the development. I had inspected the code previously before and ran away because it was a mess and required "m" and "s" , which until now I have just refused to even work with.
• next, an inspection of the code, exercises, attempts to modify. It's impossible to know how much of this is done before hand
• 3rd contribution

Since you can't measure how much of this background someone has, I'd do something more like set a bar of 200 merged PR's and you can admin. With that much work you probably know for certain good chunks of the code base, and have vested incentive to see things succeed.

[djensen47]

I prefer to see the quality of people's work over some time period in order to judge if they should be committing/merging directly to master or not.

I don't think anybody should be committing directly. Everything should be a pull request. It's just that you would be able to accept your own pull requests if you're admin.

I would hope that 20 years writing Java (yes since the beginning and yes I'm "old") would be a good qualification for quality code and architecture but ya never know. :wink:

And perhaps more importantly, I like to see what direction they are taking, what their focus is, etc, to see if they are a good fit.

For the experiences developers, yes this actually is more important. :smile:

[DanVanAtta]

Maybe there is more useful life to this thread.

I don't think anybody should be committing directly. Everything should be a pull request. It's just that you would be able to accept your own pull requests if you're admin.

I agree, except I would go further and think admins should not even be accepting their own pull requests. PR is a code review, once it is closed then it's as good as a closed ticket. Closed tickets are graveyards. So it's pointless to do a PR and not have it be for purpose of getting reviews. Once reviewed they could then hit the merge button for certain themself. (so perhaps there is no disagreement)

[ron-murhammer]

@veqryn, how do you feel about tagging what we currently have in master as either the next beta (.6) or the next release (.7)? We can they actually build/upload it when you have the time. I've essentially had that pre-release (minus your code reformatting) out for 1-2 weeks now and the one before that for a few more weeks so seems pretty stable. The bulk of the changes are around the AI. The only thing that probably should be updated is the change.log.

[djensen47]

I'd do something more like set a bar of 200 merged PR's and you can admin

IMHO that's a meaningless metric. What If somebody refactors all of the tests (over several commits) into one PR then also does a few other giant chunks of work over several weeks/months and is also great to work with? But that person never reaches 200 PRs so no admin for them! It's just silly. :tongue:

I think it just has to be done on a case by case basis with some sort of consensus by the existing admins.

[DanVanAtta]

@djensen47 , 200 is meaningless except for being quite a lot. You're not wrong. My thoughts were 200 for automatic admin (you know, give people a goal to aspire to). But in reality it should be case by case looking at a track record of contributions. In the meantime there is a labor shortage problem.

@ron-murhammer and @veqryn , with respect to tagging the current release. I can vouch for regression testing. I went through an entire 8 hour big world game with the version of master before the latest reformat, and had no trouble. With that and the AI testing others have been doing, seems liken now is a good milestone.

[veqryn]

There is some final testing that goes into a release. Namely that someone needs to go download Java 1.6 JDK and install it on a VM (that has no other java's installed) with some old version of eclipse, and then make sure that there isn't a single file with problems. I also like to run our bunch of half-or-less-coverage tests, and make sure they are all working. And of course, log into the lobby, play 1 round. Open a pbem game and play 1 turn and post it. Play 1 round against the AI by myself. Download a small map.

I hope to put my full release process in a wiki doc within the next couple days, and then we can tag 1.8.0.7 once we've verified everything is good to go.

[veqryn]

When I was talking about commits/merges, I was really talking about pull requests that are then merged. Sorry for my nomenclature, I'm still new to git. Yes, all code should be done through pull requests, because it is a hell of a lot easier to review.

I'm not really going to base admin access this on the number of commits. You guys will probably get it long before 200. Lots of accepted commits = You probably have a good vision for the project.

[DanVanAtta]

Not even sure if I even really have any minor disagreements left. Closing. Please anyone re-open if there is more to discuss.