governance: add new collaborators XIV

[Trott]

Previous round: #6613

Work-in-progress list of possible candidates with recent activity:

• @RReverser commits, comments, open PRs
• @lance commits, comments, open PRs
• @bzoz commits, comments, open PRs
• @andrasq commits, comments, open PRs
• @princejwesley commits, comments, open PRs

[RReverser]

Oh, thanks! I don't think I did much yet though, but it's nice to be considered :)

[benjamingr]

I have not had the pleasure of interacting with @RReverser in the context of Node but I had nothing but positive and pleasant interaction with him in other projects.

[rvagg]

a couple of folks mentioned in another forum that we thought @RReverser's efforts in #6893 were pretty great, both in terms of technical contribution and the heavy review process, so +1

[evanlucas]

+1 to @RReverser from me

[Trott]

Added @lance to the list of possibilities. As always, if we don't get to everyone this round, there's always next round...

[Trott]

Added @bzoz to the list.

[bzoz]

Yay, I would be happy to join!

[Trott]

Added @eugeneo to the list of possible candidates.

[Trott]

@RReverser has been onboarded. Welcome!

[jbergstroem]

Happy to see you join, @RReverser! 🎉

[santigimeno]

Welcome!

[RReverser]

Thank you! 😄

[Trott]

Questions that came up this time around where I wasn't completely sure of the status/answer:

• Do we think we might be able to set up the Merge button to work soon? (It allows you to do a rebase instead of a merge and you can edit the resulting commit message, I believe, so you can add metadata that way. I seem to recall we ran into a problem with it, but I don't remember details. Was it @Fishrock123 that was looking at it? Someone else?)
• Instead of LGTM, is there a chance we might move to GitHub sign-off anytime soon. I don't know anything about GitHub sign-off but I imagine there may be issues with making sure that our tooling still has metadata it can use. Not sure who would be able to answer this one. @rvagg or @jasnell perhaps?

[RReverser]

@Trott as for second one - I rechecked, it's only for signing off by yourself (that is, whoever lands the PR), and for other cases you still need to add people manually, so I guess not worth bothering.

First one is still interesting though.

[MylesBorins]

I don't personally see us moving to the merge button as a suggested workflow. It will always squash everything into one commit, and I have personally had mixed results with it. The alternative is not that difficult imho.

[cjihrig]

I've used the merge button a few times for really trivial things. I agree that we probably shouldn't move to it as a suggested workflow.

[RReverser]

It will always squash everything into one commit, and I have personally had mixed results with it.

Well, this would cover most of the cases as far as I understood from the guidelines. For a very specific cases when you don't want to squash, you can still use the alternative, but for simple ones this might theoretically speed up the process (hey, you can even merge PRs from your phone or whatever!).

[RReverser]

Just to be more precise - I was referring not to the old Github's Merge button behavior but to their new feature that allows to turn it to "squash / rebase".

[RReverser]

Oh, and it allows to forbid merge commits, so result should look just the same as if done in the console.

[MylesBorins]

@RReverser indeed most of our PR's are a single commit.

With that being said I've seen the merge button create commits that do not have the correct message... even if if this is a user error, it does not give us the opportunity to verify that before it lands on master. It also does not apply whitespace-fix, but that is a total nitpick.

In reality it saves at most 30 seconds of running git rebase -i HEAD~5 or git commit --ammed if you are using git am to land a patch

(you can find my alias for this here)

Now I am all for simplifying the process to an extent, but I don't think that this solves the real problem, which is verifying that commits that land on master have the appropriate meta data. It does in theory minimize the potential of pushing a commit without the meta data, as the UI will make it clear, but it does remove the stop gap where we could be linting.

TLDR; I agree that our merge process should be simplified, especially to limit error. I am not convinced the merge button is that solution

[RReverser]

Now I am all for simplifying the process to an extent, but I don't think that this solves the real problem, which is verifying that commits that land on master have the appropriate meta data.

That's surely a valid concern that I thought about too, but IIUC when landing a PR using that button you're allowed to enter all that meta data (in fact, you can change entire commit message) directly in the UI, and resulting tree should look just the same as if done from console.

Of course, I might be missing something and will be happy to learn about other concerns, just trying to find out if we can make it work for us at least in simple cases :)

[Trott]

If I understand the workflow of the GitHub button, it will probably also discourage people from running tests locally before pushing to master. I know a lot of people don't bother doing that, but I suspect a lot do. And it's definitely valuable because we've definitely seen stuff break when two commits are never tested together (especially if one of those commits introduces a new lint rule).

[RReverser]

@Trott Ah yeah, that point is scary. Can be probably covered by CI to some extent though, need to think more about it.

What about e.g. doc-related PRs?

[Trott]

I would think that it may be appropriate for single-commit doc-only PRs. @cjihrig and/or @thealphanerd probably know more about the correctness/incorrectness of that thought.

(Also, we're getting pretty far afield from the actual purpose of this issue. And that's my fault! Sorry! If we want to continue this conversation beyond another comment or two, maybe a new issue or IRC?)

[cjihrig]

Docs-only PRs that should be squashed down to a single commit and don't require any amending should be the limit. For example, I landed #6128 using the merge button. It was 7 doc commits, but squashed nicely to two sentences.

[Trott]

@lance has been onboarded! Welcome!

[lance]

Thanks! Very excited to be on board.

[Trott]

Adding @andrasq to the candidate list. Only one PR but it's a great one with lots of useful back-and-forth.

[Trott]

I had a short conversation with @eugeneo today. He intends to continue working on the inspector stuff that he and others at Google have been adding, but he doesn't feel the need to become a collaborator at this point. So for now, I've removed him from the checklist above.

[Trott]

Added @princejwesley to the candidate list.

[evanlucas]

+1 to @princejwesley. Lots of great repl work!

[Trott]

Finished onboarding with @andrasq a few minutes ago!

[andrasq]

Thank you, excited to be on board!

[Trott]

Finished onboarding with @princejwesley a short time ago!

[jasnell]

@nodejs/ctc ... can we include @joshgav on the next round! He's been an observer on CTC calls and has been doing an amazing job on keeping minutes up to date and published.

[joshgav]

@jasnell thanks! would love to come onboard, and hoping i'll be able to contribute more over time.