Node: Author ready label not used

Created on 25 Apr 2018 · 8Comments · Source: nodejs/node

It helps immensely to identify PRs that can land (or can land soon) if the author-ready label is attached.

Since it is not yet used a lot, I kindly ask all @nodejs/collaborators and @nodejs/tsc members to try to use it more frequently as described in our guide.

Most helpful comment

It’s a lot more practical to add the label just after kicking off CI, though.

Maybe we can try to change our etiquette to allow editing other collaborator’s “CI: ” comments with something like “(edit: green)” or “(edit: only known flakes)” when somebody checked the results?

addaleax on 25 Apr 2018

👍5

All 8 comments

@BridgeAR I've found that the best way to get people engage it is to ping them, it's also worth mentioning during the onboarding process.

benjamingr on 25 Apr 2018

Instead of that author-ready label, shouldn't we be using Approve less and Comment or Request Changes more?

I guess I don't really see the point of that label unless it's to distinguish a LGTM from a 'LGTM with comments.'

bnoordhuis on 25 Apr 2018

👍1

@bnoordhuis it also covers making sure the CI passed & stuff like that. It helps to not have to thoroughly check absolutely everything that was posted. The issue it's truly addressing is that not every collaborator lands commits that are not their own, there are only a handful of people that do it regularly so I think at the very least others can make that process easier.

apapirovski on 25 Apr 2018

👍1

it also covers making sure the CI passed & stuff like that.

author-ready means that the CI has started, not that it has passed.
https://github.com/nodejs/node/blob/5389c9239252b89ad2b9a52759c65800a5813baf/COLLABORATOR_GUIDE.md#author-ready-pull-requests

A pull request that is still awaiting the minimum review time is considered author-ready as soon as the CI has been started, it has at least one approval, and it has no outstanding review comments. Please always make sure to add the appropriate author-ready label to the PR in that case and remove it again as soon as that condition is not met anymore.

richardlau on 25 Apr 2018

@richardlau Thought we had changed that. Perhaps it should mean that the CI passed because that's honestly half the battle when landing pull requests. Takes me forever to check each failing test to make sure it's a known flake.

apapirovski on 25 Apr 2018

It’s a lot more practical to add the label just after kicking off CI, though.

addaleax on 25 Apr 2018

👍5

The reasons for adding the label directly after starting the CI were also discussed in the PR that changed the guide. See e.g., https://github.com/nodejs/node/pull/19116#issuecomment-370491150 (there were more comments about this).

The name of the label reflects that as well: the author is done and only we have to further agree / see that everything is fine / land the PR.

BridgeAR on 25 Apr 2018

It seems like perhaps this should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.