node-core-utils icon indicating copy to clipboard operation
node-core-utils copied to clipboard

Suggestion: auto-land and auto-release NCU Pull Requests

Open mmarchini opened this issue 5 years ago • 12 comments

Looking at our Pull Requests list, seems like we have six pull requests ready to land (with approvals), but I don't think it's clear who is allowed to land PRs in this repository. Similarly, sometimes commits will sit on master for a while until someone has time to publish a new version on npm. My suggestions:

  1. A scheduled GitHub Action that will merge all PRs with at least one approval and green CI. This could run every 5 minutes (quickest the scheduler can run), or maybe every hour (since we don't have a big flow of PRs here).
  2. A scheduled GitHub Action that will make a new release to npm every day (if we want quick releases) or every week (if we want slower releases)

We would need to identify semverness of PRs somehow, either via labels or using https://www.conventionalcommits.org/en/v1.0.0/. Looking at our logs, we're already not strictly following Node.js core commit guidelines and we have mixed Node.js and conventional commit messages.

As for npm releases, it might be tricky because we require 2FA. If anyone worked with 2FA and automated npm releases before, I'd be happy to see solutions :)

mmarchini avatar Jun 27 '20 19:06 mmarchini

@mmarchini I definitely agree that there's a bit of a bystander effect going on with who has authority to merge things, but i'm not 100% sure that automatic merges are the right solution here for a few reasons:

  1. Fairly few people (i think it's basically ~4-5 of us atm?) maintain this repo, so it's often the case that 1-2 specific people's approval is needed for a given PR as they're the one(s) who'll have the necessary context
  2. CI on this repo is a bit of a struggle bus at present especially on macOS - I see it fail/flake all the time on PRs like this one

What do you think of perhaps adding CODEOWNERS? I think that might make ownership areas a bit more straightforward 🤔

For the latter suggestion, I think weekly releases would be my personal preference but i'd also be ok with daily if that's the consensus from others!

wombat-dressing-room is the best solution to the 2FA problem i can think of off the top of my head - Electron also built a similar thing with https://docs.continuousauth.dev/ but it integrates with Slack so that's probably not ideal for our situation here.

codebytere avatar Jun 28 '20 03:06 codebytere

CODEOWNERS doesn't solve the issue of who can land PRs (the access tab is a mix of individually granted permissions and teams, and it's not clear which team should be pinged to land things here), only who has context to review PRs. It's a great feature to add to the repository, but it wouldn't solve the lingering already-approved PRs issue.

The scope and user base of this project is limited enough that I think daily releases wouldn't be harmful, and fixes would get out faster without needing for a person to start it (unless there's a critical issue which needs to be released right away). I think getting improvements out for collaborators faster would benefit the project. But I wouldn't mind weekly releases either if that's what folks prefer.

The flaky CI is definitely an issue, although if it is a small subset of tests that fail infrequently maybe an automerge would at least offload some PRs?

Thanks for sharing Continuous Auth! I'll definitely take a look.

mmarchini avatar Jun 28 '20 04:06 mmarchini

I've seen that Electron uses Semantic Release for a few of their modules and it seems pretty handy in terms of auto releasing as new features land. Potentially something to consider?

https://github.com/semantic-release/semantic-release

bnb avatar Jul 10 '20 18:07 bnb

As a start we could add https://github.com/googleapis/release-please or https://github.com/semantic-release/semantic-release to just create the changelog and release commit automatically (not sure how semantic-release works, release-please will open a PR and will update it every time a commit lands until someone merges it, which is good if we'll publish to npm manually for now). Both follow https://www.conventionalcommits.org/en/v1.0.0/ afaik.

mmarchini avatar Jul 22 '20 18:07 mmarchini

Was looking at this again after #488 landed. wombat needs to run on GCP, so we would need donated credits to use it on the Node.js org. CFA doesn't work with Actions yet, which is a blocker for us (I don't think requiring Slack is a problem, we could probably use the Foundation Slack). I'm not aware of any other options. While CFA looks more promising, I'm not sure how it fares on multiple projects with multiple users (assuming we want to enable it to any Node.js project). Would it be possible for a subset of folks to be allowed to release node-core-utils, and a different subset to be allowed to release llnode, for example?

mmarchini avatar Aug 25 '20 01:08 mmarchini

@nodejs/node-core-utils would you be willing to try https://github.com/mmarchini-oss/npm-otp-publish to autopublish when we merge release-please PRs (since I didn't add a README yet, see https://twitter.com/mmarkini/status/1299202760911405060?s=20 for a summary of how it works)? I'm still working on it, but it should be usable as it is today. node-core-utils could be one of the first projects (outside my personal ones) to try it :). The only downside is that we would need an npm bot account and we would need to share the OTP of that account with those of us who want to publish, but it shouldn't be that big a deal IMO.

mmarchini avatar Aug 31 '20 01:08 mmarchini

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

github-actions[bot] avatar Nov 30 '20 00:11 github-actions[bot]

@mmarchini also worth noting there's now npm automation tokens. Not sure if they'll be a good solution for the specific use case (I wouldn't be surprised if they're not - there's still work going into them to make them more broadly usable), but worth a mention at the very least.

bnb avatar Dec 30 '20 18:12 bnb

I think @targos set up token in this repo?

mmarchini avatar Jan 04 '21 22:01 mmarchini

What token? I don't remember

targos avatar Jan 05 '21 05:01 targos

https://github.com/nodejs/node-core-utils/pull/511

mmarchini avatar Jan 05 '21 22:01 mmarchini

Ah, I did not setup the token yet. I don't have access to the bot's npm account.

targos avatar Jan 07 '21 17:01 targos