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

Switch to Changesets-based release process #118

Merged
merged 9 commits into from
Nov 25, 2021
Merged

Switch to Changesets-based release process #118

merged 9 commits into from
Nov 25, 2021

Conversation

Andarist
Copy link
Member

The goal of this PR is to have a good release process in place. After this lands we should:

  • have a clear way to pin the action to a specific version
  • should no longer have guaranteed conflicts when parallel work is happening on branches (cause dist wont be committed now)
  • should increase the security (we were not reviewing dist before merging and a bad actor could tamper with this)

on:
push:
branches:
- main
Copy link
Member Author

Choose a reason for hiding this comment

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

we should be able to rename the base branch now - we need to preserve master as artifact of the past though as all existing users are using that ref at the moment

@@ -0,0 +1,35 @@
const path = require("path");
Copy link
Member Author

Choose a reason for hiding this comment

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

I've done my best to test this before creating the PR - but as with any CI work... i could miss something and we'll have to supervise the first automated run of this script

@@ -0,0 +1,35 @@
const path = require("path");
const { exec, getExecOutput } = require("@actions/exec");
const tag = `v${require("../package.json").version}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

There is also a question - what version should we release first? I probably would say that we should immediately bump to v1 - leaving a possibility to add all missing tags retroactively in the future

Copy link
Contributor

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Great idea, thanks for looking into this!

- name: Checkout Repo
uses: actions/checkout@master

- name: Setup Node.js 12.x
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use at least node 14?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if that has changed since then but apparently noode 14 couldn't be used a few months back:
actions/github-script#182 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is actions/github-script being used?

Copy link
Member Author

@Andarist Andarist Nov 25, 2021

Choose a reason for hiding this comment

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

ah, i might have confused both things in a hurry - it might only not be possible to use node 14 for action itself but a workflow through setup-node action can actually use node 14

.github/workflows/version-or-publish.yml Outdated Show resolved Hide resolved

- name: Install Dependencies
run: yarn

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a step to build the code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

build is happening within the release script - and it's only done after we check if we need to release at all, so this is a minor optimization of the execution run, since most of the runs wont need to release and thus wont have to run build

.github/workflows/version-or-publish.yml Outdated Show resolved Hide resolved
Comment on lines +27 to +28
version: yarn version
publish: yarn release
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why do we need those two custom built scripts? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

version:

  • runs changeset version
  • and updates README

release:

  • checks if the current version has already been released
  • builds
  • commits
  • tags the commit
  • pushes the tag out

It's not a standard npm-based flow that is somewhat assumed by Changesets.

@emmatown
Copy link
Member

Could we add a v${major} tag? (and maybe v${major}.${minor} but I don't care very much about that) I don't really see the value in setting changesets/action@v${major}.${minor}.${patch} since it's bad for both staying up to date and ensuring the code you're using can't be tampered with, if you want to ensure that the code can't be tampered with, you should be using a commit and if you want to stay up to date, you should use a v${major} or v${major}.${minor} tag.

@Andarist
Copy link
Member Author

@mitchellhamilton Ok, makes sense - I've implemented the suggested change

Comment on lines +36 to +42
await exec("git", [
"push",
"--force",
"--follow-tags",
"origin",
`HEAD:refs/heads/${releaseLine}`,
]);
Copy link
Member Author

Choose a reason for hiding this comment

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

note that i've chosen to use a branch for this because branches sync better than tags between remote and a local copy, to push out a tag with the same label we need to first destroy it and recreate. In general, I prefer to treat tags as immutable pointers.

@emmatown
Copy link
Member

The code looks fine to me, I'm thinking we should merge this to a new main branch, do a release and when we've tested it, change the default branch to main so we can make sure it works without breaking existing users if the new thing is initially broken? After that, we might want to add to the action on master to make it log a warning (as in https://docs.github.com/en/actions/learn-github-actions/workflow-commands-for-github-actions#setting-a-warning-message) telling people to use the updated version.

@Andarist
Copy link
Member Author

@mitchellhamilton do u want to do it or should i go with it when i wake up?

@emmatown
Copy link
Member

Happy for you to do it

@Andarist Andarist changed the base branch from master to main November 25, 2021 16:24
@Andarist Andarist merged commit 05c863d into main Nov 25, 2021
@Andarist Andarist deleted the use-changesets branch November 25, 2021 16:51
@github-actions github-actions bot mentioned this pull request Nov 25, 2021
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.

3 participants