-
Notifications
You must be signed in to change notification settings - Fork 919
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
Conversation
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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
- The git tag created needs to point at a commit on
master
, without that the canary prereleases can't correctly infer their version number - 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:
- Make a PR that updates the changelogs and merge it, this is somewhat unrelated to the actual versioning and release
- Make and merge a PR that captures the changed files from
pnpm lerna version --no-push 7.0.0
- 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.
There was a problem hiding this comment.
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:
- ...
- Make and merge a PR that captures the changed files from pnpm lerna version --no-push 7.0.0
- 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:
- contributor creates remote branch turf/releases/7.0.0, and checks it out locally
- 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 ... - contributor creates a PR to merge turf/releases/7.0.0 back in to turf/master, honouring branch protections
- 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.
There was a problem hiding this comment.
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
Will try a few more mock releases tomorrow, including a series of alphas to make sure it holds up.
There was a problem hiding this comment.
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.
% 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.github/workflows/release.yml
Outdated
push: | ||
tags: | ||
- "v*.*.*" | ||
types: [published] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I disabled the prerelease action for now, so we can land this and release 7 before re-enabling prereleases later if we want.