Make release idempotent (+dry-run)

[NullVoxPopuli]

I removed the workflow that does the automated release, because we need a couple things first:

• merge queue (mitigate the problem of different PRs not being totally rebased at the time of merge, and not being tested with one another post-merge)
• the .release-plan.json needs to have a sha in it, so we can check out the copy of the code that should contain the code-to-be-released
  • this can mean that main gets far-ahead of whatever is to be released, but all we need to do is update the release-plan

---

This idempotency will allow us to run the release command on main without worry of errors or anything like that.

Blockers (these are all dealt with now)

• preparing patch 3.1.1 #1490 (review)
  • need to make the paths there repo-local
  • PR here: Make the package-json path repo-relative #1500

Sample output:

❯ pnpm i
❯ pnpm compile
❯ pnpm exec embroider-release publish --dryRun --skipRepoSafetyCheck

 ℹ️  The tag, v3.1.1-compat, has already been pushed up for @embroider/compat
 ℹ️  --dry-run active. Skipping `git push --tags`
 ℹ️  A release with the name 'v3.1.1-compat' already exists
 ℹ️  @embroider/compat has already been publish @ version 3.1.1
 🎉 --dryrun active. Would have successfully published release! 🎉

There are 4 steps to release:

• create tags
  • this now first checks if a tag exists, and if it does, logs, skipping the creation of the tag
• push tags
  • push tags doesn't error when there are no tags to push, so there is nothing to guard against here

    ❯ git push --tags
    Everything up-to-date
    
    ❯ echo $?
    0
    

    however, since, when testing this, I didn't want to actually modify anything, I added a --dry-run flag so I could see (via logs) that either something {con,de}structive would happen, or if it'd be skipped via the idempotency checks
• make release on github
  • this now checks if the release already exists, and if it does, logs, skipping the creation of the release
• publish to npm
  • this now checks if the package at the proposed "newVersion" is already published, and if it is, logs and skips publishing

[mansona]

All in all a good PR, I'm a bit confused by a few things in the release.yml and they should probably get sorted before merge 🤔

[mansona]

why are we explicitly using process.stdout.write when in other places in this same PR we're relying on the console functions?

[NullVoxPopuli]

pre-existing choice, idk

[mansona]

also why are we prefixing everything with \n instead of just making sure that everything ends with an \n?

[NullVoxPopuli]

So that it stands out -- error messages are important, and we aren't giving them any color or anything, so they need another means of standing out.

[NullVoxPopuli]

Open questions from the call:

• how do we handle the case where this accidentally fails, and more merges occur, which then makes "the build green", and then we accidentally release stuff not described in the plan?
  • we probably need to include the sha that the release-plan was generated for, and then add some logic to embroider-release publish to abort if there are un-planned changes (merges since the plan was created)

[chancancode]

It seems like this workflow is intended to run on every push to main, and use relying on the script being idempotent if stuff has already been published?

It does seem problematic to release whatever happens to be on master, especially given that:

• The workflow doesn't seem to wait for the tests
• Presumably some human at one point decided to release a certain set of changes (generated changelogs, made the release plan, etc)

In other places where we (I?) have set up automated publishing, we generally accomplish it by

1. a human take some action locally (on their computer), then
2. publish those Actions to GitHub, then
3. GitHub Actions does the publishing in CI in response to the "trigger"

Specifically, one way that I typically set this up is:

1. do whatever you need to prep the release (generate changelogs etc) and get those pushed up and merged
2. wait for all the commits you pushed up passes tests on CI, etc
3. locally, git tag the commit you want to release (e.g. git tag v2.3.4)
4. git push --tags
5. the publish workflow is set up to run in response to tags matching v* (only) and publish the release (to NPM, GitHub, etc)

Since the publish workflow only runs on tag pushes and is tied to the SHA that the tag is for (inherently, you'd have to go out of the way to checkout a different SHA on GitHub Actions), it will never "publish the wrong things".

In this case, I can appreciate that tagging may not be the best "trigger" because it's a monorepo, etc, but I don't see why you would need to forgo the general concept here and have the script run on main on every push, which does indeed have the problems you described.

Some alternative triggers that would work:

• only run the workflow on the main branch if the changeset touches the release plan
• only run the workflow on pull request merge if the pull request has a specific label applied to it
• a manual workflow_dispatch that takes a SHA as a required parameter
• a special GH comment on a commit

In any case I think the issue is that the workflow runs too often/too willy-nilly, whereas you probably want it to be a lot more deliberate IMO.

Separately to the issue you brought up, there is also another related race condition of using a PR-based workflow for releasing (as opposed to a direct git push workflow) – in between the time you authored the content for the release PR (generated changelogs, etc) and merging the PR, someone else could have merged other things, which could inadvertently go out with the release against the release PR authors intentions:

1. The author of the PR announce their intention to cut a release, as of the latest commit they are aware of when creating the PR, which actually is known to git/github (the "base ref" of the PR)
2. The approver of the PR concurs with the intention to cut a release (but it's unclear what they think they are releasing, since GitHub doesn't make it obvious what a PR's base ref is
3. The person who clicked the merge button (maybe one of the same people) concurs with the release, but again unclear what they think is being released
4. In any case, when the merge button is clicked the button, you are essentially "rebasing" the release plan against the latest content of the main branch

So I think using a PR workflow for releasing is generally a bit problematic for this reason (as compared to "push the tag"). You are adding an extra layer of review, but at the cost of introducing the race condition and it's unclear what the reviewers are really reviewing/approving.

You could probably enforce this socially by asking people to hold off on merging things on discord, which perhaps works good enough.

The more correct solution probably requires margin a release branch, and you are PR-ing against that branch. Done correctly it may even allows you to review all the changed files in the release, but it does have its own set of issues (the git tag would not point to a commit that exist on main, if you care; you have to have more branches if you want to do backports and each time it requires non-trivial setup).

[NullVoxPopuli]

I just removed the publish.yml, as I don't want how we publish to block the work here of

• dry-run
• idempotent release

these are still useful things to have for local publishing.

---

@chancancode I understand your concerns, but the goal here is to remove the manual work of traditional releases (tagging, etc).
Other ecosystems do this quite well (changesets being one of them, (we don't use it because we didn't want to create changelog entries at merge time, pnpm -r publish is very good, we wanted to use lerna-changelog-tagging, and we have custom additional Changelog needs)).

But, my take on some things you've mentioned:

The workflow doesn't seem to wait for the tests

tests pass before merge -- which is usually sufficient -- once we enable the merge queue (which I don't think we've talked about yet, as a team), it would eliminate all potential risk with double-running the tests before a release. The issue now is what the merge queue would solve, multiple PRs from different points in history can be added to the head of main, but they haven't been tested with each other yet -- so running the release here (without the aforementioned sha check) is quite risky!

Presumably some human at one point decided to release a certain set of changes

yeah! that's the .release-plan.json in the root of the repo

that tagging may not be the best "trigger" because it's a monorepo

exactly, we have a tag per-package per-version.
Additionally, the format of tags is very specific, and if anything is manually done, that's prone to human-error.

but I don't see why you would need to forgo the general concept here
and have the script run on main on every push,

This'll be for the next (or oxt) PR, (where I bring back the workflow), but given:

• merge queue
• checking out a specific sha in history, rather than running on main

The risk of anything going wrong is quite low.

only run the workflow on the main branch if the changeset touches the release plan

• this is good in general, but potentially too restrictive, a manual trigger would be needed as well (probably a good thing to have anyway)

• only run the workflow on pull request merge if the pull request has a specific label applied to it
• a special GH comment on a commit

seems like extra work / process (more manual / prone to human error stuff)

a manual workflow_dispatch that takes a SHA as a required parameter

we can forgoe the sha by having the sha in the .release-plan.json

that the workflow runs too often/too willy-nilly

On most of my projects, I have Changesets running in the same manor, and it's no issue.

in between the time you authored the content for the release PR (generated changelogs, etc) and merging the PR, someone else could have merged other things, which could inadvertently go out with the release against the release PR authors intentions:

providing the sha-to-checkout in the .release-plan.json mitigates this problem, and reduces the need for manual processes.

So I think using a PR workflow for releasing is generally a bit problematic for this reason (as compared to "push the tag"). You are adding an extra layer of review, but at the cost of introducing the race condition and it's unclear what the reviewers are really reviewing/approving.

tag pushing is not viable, but everything you've suggested is at least double the amount of manual review / approving. 🙃

I really do appreciate the time you took to write out all these concerns though.
I think they'll be addressed with time, and we 100% don't want to active automatic publishing before the concerns are addressed!!

Thank you!!!

[chancancode]

hm, it seems like you main takeaway was that I think we need to increase the amount of manual steps, or increase the places where things would go wrong, but I don't think that's quite what I am saying, and in fact the same core goals are definitely shared.

anyway, happy to chat about this more elsewhere if that would be helpful, but it sounds like you already have concrete ideas for solving the problems you are facing, so all is good

[NullVoxPopuli]

Apologies for misunderstanding.

We can chat when i'm ready to create the workflow that will run the automation!

[mansona]

LGTM 💪