How we manage things in GitHub - PRs

Product Development Process
1

Continuing the discussion from How we manage things in GitHub - labels and How we manage things in GitHub - Milestones:

This is the final of the 3 topics that I’ll add, and it covers off how we’d like to manage PRs through the pipe to merged and released.

Here’s a drawing of the process, my attempt to help visualise the steps in the process:

IMG_0466.jpg3719×1557 1.19 MB

As you can see it’s a 6 step process, from creating the PR through to merging it.

Some things to note in it that are new:

Use of labels

We would like to create a set of labels specifically to be used for PR management. Here is the full list:

pr-wip - use when you’ve created a PR but it isn’t ready to go through the process yet.

pr-no-review - use when your PR doesn’t require a code review or testing (e.g. Transifex branches required for releases).

pr-no-test - use when your PR doesn’t impact on functionality and doesn’t require any front end testing.

pr-test-ready - use when your PR has been reviewed and is ready for testing.

pr-ready-to-go - use when your PR has been reviewed and tested and is ready to be merged.

aus review - (existing) use when you want the AU team to look at your PR at any point in the process.

Obviously, as you move your PR along the process you remove these labels when they are no longer relevant.

Use of entry and exit criteria for each step

You can clearly see as you create and move things through the pipeline exactly what you need to do, who is involved, and what is required to be able to move it to the next step. It’s up to you as someone developing code on the platform to make sure you follow these steps, and ensure that they’re being following by others. It makes it easy for us non-devs to also understand where things are at and whether we need to look at something or not. Win-win!

OK, so now it’s over to you all for comment. Ping @MyriamBoure @sigmundpetersen @lin_d_hop @Matt-Yorkley @sstead @NickWeir @Kirsten @enricostn @sauloperez. I’ll give this more time for comment, and will look to implement the results of our discussion into GitHub in the week commencing November 6 alongside of making other changes to the labels :slight_smile:

3 Likes
2

Wouldn’t it be great as an international community if we now could turn aus review into pr-review? :smiley:
Since there are now also expert reviewers outside Aus.

Or is aus review also used for other things? I welcome a discussion around this label.

Other than that this is another great streamlining proposal from the magnificent Australia team! :raised_hands:

3

Agree with @sigmundpetersen. We also want to raise the discussion on whether we, katuma team, could also test some small PRs.

What about having both aus review for PRs that deserve more detailed and experienced QA and pr-review for small things that can be tested by the same instance that opened the PR? As a result, quick and easy PRs won’t depend on timezones. This requires renaming pr-review to something else or it’ll clash with the 2nd stage of the diagram

4

What about differing on the type/level of the review requested? Something like pr-review-full and pr-review-express or similar :slight_smile:

5

Yep, that could do the trick.

6

Just a couple of things in response to this idea:

  1. aus review was set up to be something bigger than just code reviews. It was a means to get the AU team’s attention, whether it’s a code review, testing, a question about functionality, whatever the need and be it on a PR or an issue. It’s a way to get the attention of what was (and still is, for the moment at least) the people with the most knowledge of the code base and functionality. So I don’t think we should change this from what it is.

  2. The process that I’ve outlined here shows that it is possible for a PR to go through and be merged without needing the AU team involved. Firstly, because @enricostn is one of the 3 who must have reviewed a PR before it goes through to testing, and who can merge to master. And secondly, because anyone can test, so long as a) there is recognition that if something is complex we get an expert to look at it, and b) they add testing notes to the PR/issue.

  3. This process is already happening (though there were no testing notes added to the PR) with a PR that Katuma developed, reviewed, tested and merged. Moving forward, this can still continue to occur and I won’t be asking whether something was tested (as I had to at the time because I couldn’t see that just by looking at the PR) because you will have added testing notes and it will be obvious :smiley:

So given the above I don’t believe we need the suggested labels of pr-review-full and pr-review-express. But I would be keen to hear if you all agree, especially @sigmundpetersen and @sauloperez :slight_smile:

7

No worries, I support the suggestion as is. Thanks for the explanation, I understand better now :smiley: (No need for extra PR labels)

1 Like
8

Thanks @danielle for this clarification proposal ! So from my understanding a local instance is free to test any PR with the label pr-test-ready and any authorized dev (Enrico, Rob, Maikel, Rohan and Lynne so far) is free to merge any pr-ready-to-go labelled PR.
So a local instance can just manage his own processes locally if they want. And we only use Aus review if we want feedbacks on anything from the Aus team.

I just have one suggestion: I am also more and more tempted, in the French instance, to ask feedbacks and review to @enricostn for instance, who is in the same timezone as we are so if Pierre is blocked on something I would like a label more generic than “Aus review” but more “experts review”, so that the first expert available can work at it (I know today it’s mostly Aus but hopefully more people will become experts!). If I put the label on Friday and no dev is at office in Aus before Wednesday, I’m happy if some other experts can give a feedback. That would contribute to unlock some “funnel” effect that are sometimes hard to deal with due to timegap but also to days of work / work capacities of the Aus experts that are over sollicitated ;-).
We could even just use the label “blocked” which means we are unable to go further, whether it be for merging (we have no local dev authorized) or inputs on some tech difficulties, so it would be great that the pool of experts (in Aus but also Enrico for instance) could have the role to “unblock” those who are blocked. In that case we only need the “blocked” label and the Aus team instead of looking at “Aus review” label would look at all “blocked” issues.

Side comment : Of course I’m also happy if Enrico picks up “blocked” issues if he can be paid for his time as Aus reviewers are at the moment… can be sometimes on CC, sometimes on projects paid by instances… hoooo, I guess we gonna need a common time tracking tool soon or it’s going to be a mess…

Anyway, if you want to keep going for now with the Aus review label @danielle I have no objection as, as you said, most code experts are still there, so you can ignore my suggestion if it doesn’t seem relevant… Cheers !

9

Thanks for the input @MyriamBoure :slight_smile:

a local instance is free to test any PR with the label pr-test-ready

Yes, as long as they have people who understand how to test, and what to test and you trust that they know what they are doing. However, if it’s complex functionality (e.g. email confirmation, or stripe, or subscriptions) then we would want our expert testers to also test to be certain that we’ve picked up as many problems as we can.

any authorized dev (Enrico, Rob, Maikel, Rohan and Lynne so far) is free to merge any pr-ready-to-go labelled PR

At this stage @lin_d_hop and @enricostn are merging smaller PRs that don’t have much impact. At present when the AU team merges to master we also push to our Australian production. This means that if something has been merged by Enrico/Lynne the first time it’s “tested” in production is when an AU dev pushes a PR. We’ll be changing this to decouple master from AU production once @oeoeaio has some time, but for now we feel safer having the more complex things merged by an AU dev. Perhaps Rob can add to this explanation better than I can, as we both developed this approach so input from him would be valuable at this point in time :smiley:

What you can do, is have enrico review the PR (and you can pay him for that work directly, similarly to how you pay AU devs), and test it yourselves on your own staging. Then the only thing you need AU help on is a final quick once over of the code, the merge, and the review on AU production.

We definitely want to keep aus review for now, because as I said before it isn’t the label to put on when you move something to code review or test ready, it’s purely to get our attention if you want/need us to do a review, or to test, or to answer a question.

We could even just use the label “blocked”

The blocked label works really well at saying this PR/issue can’t be progressed due to either another issue/PR needing to be done first, or because the solution is unknown. I don’t think we should extend the definition of this to include that it’s been moved from one step to the next and needs a review. IMO I would rather an additional label calling for a code review, than to use blocked.

I guess we gonna need a common time tracking tool soon or it’s going to be a mess…

That would be awesome :smiley:

We can chat more about all this in December when we’re all together, and it will no doubt all change then. I think this works as a great interim solution until this time.

Cheers!

10

@danielle would it be an option to just rename aus review to something like aus attention, to remove the ambiguity towards code review?

11

Sure, sounds like a good idea :slight_smile:

12

I understand the need for AU to test before deploying, especially if it goes directly to production. As I said on Slack the problem I would like to solve by evolving some processes is a “time” problem. Having 2 reviews and test as you suggest (Enrico + Aus) might not make this quicker :wink: But we are also discussing all that with @Rachel and will fuel some concrete proposals for our discussions in Aus !

13

Label is now changed to be aus eyeballs …because that’s way more fun that attention and it’ll make me chuckle each time I see it :laughing:

1 Like