Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Author ready label not used #20282

Closed
BridgeAR opened this issue Apr 25, 2018 · 8 comments
Closed

Author ready label not used #20282

BridgeAR opened this issue Apr 25, 2018 · 8 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@BridgeAR
Copy link
Member

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.

@BridgeAR BridgeAR added the meta Issues and PRs related to the general management of the project. label Apr 25, 2018
@benjamingr
Copy link
Member

@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.

@bnoordhuis
Copy link
Member

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.'

@apapirovski
Copy link
Member

@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.

@richardlau
Copy link
Member

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.

@apapirovski
Copy link
Member

@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.

@addaleax
Copy link
Member

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?

@BridgeAR
Copy link
Member Author

The reasons for adding the label directly after starting the CI were also discussed in the PR that changed the guide. See e.g., #19116 (comment) (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.

@Trott
Copy link
Member

Trott commented Nov 6, 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.

@Trott Trott closed this as completed Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

7 participants