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

Prep infra for v7 release #2607

Merged
merged 8 commits into from
May 17, 2024
Merged

Prep infra for v7 release #2607

merged 8 commits into from
May 17, 2024

Conversation

mfedderly
Copy link
Collaborator

  • Add a new release workflow and associated package.json script that mirrors prerelease but for a proper release. This should trigger when a github release is published.
  • Update CONTRIBUTING.md based on my best guess of how the release should work once this PR merges
  • Change the prerelease script from major to minor prereleases

I disabled the prerelease action for now, so we can land this and release 7 before re-enabling prereleases later if we want.

package.json Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
- make a commit containing any last minute changelog entries
- make a commit containing the changes from `pnpm lerna version --no-push 7.0.0`
- create a PR from your branch and merge it
- use the github UI to create a release with a new tag with a matching version (v7.0.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the best way to go.

Create a new branch - presumably in my fork
Commit last minute changes - and push to my new fork branch?
Run lerna version locally - but don't push package.json updates or tag?
Create a PR (without package.json updates or tag?) and merge - fork branch would only have last minute changelog entries though?

Also, github release can use a pre-existing tag. Why not let lerna version tag the release? Feels like creating tags two different ways is making it hard on ourselves.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ideally I don't want the user touching lerna commands at all. I did some more reading today and I think we might be able to just make a runnable workflow that takes the tag as the target and just does all the magic itself.

Let me try that today and see how far I get. We might have to try a few times to get it to work properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure that I can make the powerful releasing action described above. I don't seem to be able to create a Github PAT for the Turf organization at all, and if we wanted to have it be able to bypass branch protection we'd need to have a separate user or Github application which is complicated and hard to guard against bus factor.

There's a few constraints that I think are important for the release workflow:

  1. The publish step should be done by CI, this lets us leverage the NPM_TOKEN and gives us some confidence that the builds are being done officially
  2. The git tag created needs to point at a commit on master, without that the canary prereleases can't correctly infer their version number
  3. Branch protection on master should prevent anyone from unilaterally pushing commits and should stay enabled as much as is feasible

Because of branch protection, I don't think we have the ability to make changes to the package.json files from within Github actions. That means that the package.json change must be done by a human and submitted through PR.

Additionally, the commit that updates the package.json changes cannot be automatically tagged as part of the same lerna version step, because that creates a tag pointed at the PR branch instead of a commit on master, and therefore Lerna cannot use it to infer the next canary version. lerna version supports looking for tags on merged commits, but lerna publish does not seem to have an option for this.

So my proposed workflow for creating a release right now does require some redundant manual steps, but it gets us unblocked for now and we can maybe revisit this to improve things going forward with a tool like Changesets that doesn't require bespoke infrastructure.

So in summary my proposal to release v7 would have the following steps after merging this PR:

  1. Make a PR that updates the changelogs and merge it, this is somewhat unrelated to the actual versioning and release
  2. Make and merge a PR that captures the changed files from pnpm lerna version --no-push 7.0.0
  3. Create a release using the Github UI with a matching version number which triggers the actual package publish to NPM

Its regrettable that steps 2 and 3 are somewhat redundant and manual, but it is an upgrade from the previous process and unblocks us for now/v7. The only dependency that this process has on me specifically is the Github secret that contains the NPM publishing token, which could be replaced quite easily by anyone else that has permissions to the NPM organization. I don't see another way to navigate all of these constraints today without making the infrastructure more complicated and more dependent on a single person.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in summary my proposal to release v7 would have the following steps after merging this PR:

  1. ...
  2. Make and merge a PR that captures the changed files from pnpm lerna version --no-push 7.0.0
  3. Create a release using the Github UI with a matching version number which triggers the actual package publish to NPM

The last two steps just feel awkward. They can't be the right way to go.

How about we totally ignore canary builds for the time being:

  1. contributor creates remote branch turf/releases/7.0.0, and checks it out locally
  2. contributor runs lerna version major --ignore-scripts --yes locally, lets lerna make the changes, create the tag, and push them all automatically to the remote. If it isn't what we expect, we can just delete the turf/releases/7.0.0 branch. If it all looks as expected ...
  3. contributor creates a PR to merge turf/releases/7.0.0 back in to turf/master, honouring branch protections
  4. contributor uses UI to create a release based on tag pushed above, triggering release build on CI infra and publish to npm. We would have to add --include-merged-tags to the publish command first though

Maybe that's as complicated as it needs to be?

Thought - do we want to set up a skeleton repo to test some of these things out? We can publish temporarily to a nonsense NPM namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to put my money where my mouth is. Set up a test repo and tried the process above. Early signs are it works fine. I didn't even have to merge back to master first (step 3) - just created the release.

If you want to take a closer look:

github: https://github.com/smallsaucepan/jhqqql
npmjs: https://www.npmjs.com/search?q=%40jhqqql

Screenshot 2024-05-15 at 10 49 16 pm

Will try a few more mock releases tomorrow, including a series of alphas to make sure it holds up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its relatively easy to get it to publish packages to the npm repo, the workflows let us be really flexible with what triggers a release and what branch it lives on. However, the current state of that repo has a few problems now.

The first is that without merging releases/1.0.0 back into master, the package.json files don't have the right version, which could get confusing if we allow it to get increasingly more stale.

The second is that the --canary versions are now inferred incorrectly. I don't think that merging release/1.0.0 into master fixes this. Keep in mind that for a clean repro of what merging the release branch into master would look like, you should use the Github UI with either Create a merge commit or Squash and merge because that's what we currently have enabled in turfjs. You might want to try both ways, but I definitely had to delete a 6.5.0 tag and recreate it to get the canary builds working for the 7.0 prereleases because lerna didn't see the tag which had been pointed to the branch instead of master.

image

% pnpm lerna publish minor --canary 
lerna notice cli v7.4.2
lerna info canary enabled
lerna info Looking for changed packages since 05c8b8d^..05c8b8d

Found 2 packages to publish:
 - @jhqqql/aaa => 0.1.0-alpha.3+05c8b8d
 - @jhqqql/bbb => 0.1.0-alpha.3+05c8b8d

^ these should be 1.1.0-alpha.something

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first is that without merging releases/1.0.0 back into master

Understand that. So we just merge it. It's not that onerous a housekeeping task.

I don't think that merging release/1.0.0 into master fixes this ... you should use the Github UI with either Create a merge commit or Squash and merge

It does seem to, if the --include-merged-tags flag is used so Lerna can see the tag. As requested I tried both approaches:

After squash and merge back to master (now at v1.0.0):

$ pnpm lerna publish minor --canary --include-merged-tags
lerna notice cli v7.4.2
lerna info canary enabled
lerna info Looking for changed packages since 070310d^..070310d

Found 2 packages to publish:
 - @jhqqql/aaa => 1.1.0-alpha.4+070310d
 - @jhqqql/bbb => 1.1.0-alpha.4+070310d

After merge commit back to master (now at 2.0.0):

pnpm lerna publish minor --canary --include-merged-tags
lerna notice cli v7.4.2
lerna info canary enabled
lerna info Looking for changed packages since 72203f4^..72203f4

Found 2 packages to publish:
 - @jhqqql/aaa => 2.1.0-alpha.1+72203f4
 - @jhqqql/bbb => 2.1.0-alpha.1+72203f4

Anyway, I'll finish reviewing the PR. You're welcome to proceed as you see fit.

Copy link
Collaborator Author

@mfedderly mfedderly May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what I was missing... --include-merged-tags isn't on the Lerna publish help, only version. Glad it works though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smallsaucepan let me know what you think of the changes, I reworked the docs/procedure it to reflect that the user can push tags. We can have the workflow create the Github release for us automatically.

push:
tags:
- "v*.*.*"
types: [published]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the types: line left over from the release: clause that used to be in there? Not sure it goes with push: Try it, though might be worth checking if the workflow barfs or doesn't trigger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, meant to remove this

@mfedderly mfedderly merged commit 1f39da5 into master May 17, 2024
3 checks passed
@mfedderly mfedderly deleted the mf/7-prep branch May 17, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants