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

DEV - Refactor release CI workflow #425

Merged
merged 18 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions .github/workflows/build.yml

This file was deleted.

4 changes: 2 additions & 2 deletions .github/workflows/javascript-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ on:
pull_request:

jobs:
build:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the name because it was confusing seeing this in the UI and on my terminal when tracking CI runs

js-tests:
runs-on: ubuntu-latest

strategy:
Expand All @@ -23,7 +23,7 @@ jobs:
steps:
- name: "Checkout repository 🛎"
uses: actions/checkout@v4
- name: Setup Node.js ${{ matrix.node-version }}
- name: "Setup Node.js ${{ matrix.node-version }}"
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
Expand Down
107 changes: 81 additions & 26 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
@@ -1,58 +1,113 @@
name: Release conda-store-ui
name: "Build and maybe release conda-store-ui"

on:
# we want to make a release whenever a new tag is created
release:
types: [published]
push:
tags:
- "*"
branches: [main]
tags: ["*"]
pull_request:
branches:
- main
workflow_dispatch:

jobs:
call-build:
uses: conda-incubator/conda-store-ui/.github/workflows/build.yml@main
env:
FORCE_COLOR: "1"
PACKAGE_FILE: "conda-store-ui.tgz"

make-release:
jobs:
# always build and verify
build-application:
name: "Build conda-store-ui"
runs-on: ubuntu-latest
# ensure that the artifacts are available from the build job
needs: call-build

steps:
- name: "Checkout repository 🛎"
uses: actions/checkout@v4

# Setup .npmrc file to publish to npm
- name: "Set up Node.js 🧶"
uses: actions/setup-node@v4
with:
node-version: 18
registry-url: "https://registry.npmjs.org"
scope: "@conda-store-ui"
node-version: "20.x"
cache: "yarn"

- name: "Install dependencies 📦"
run: yarn

- name: "Lint code 🔎"
- name: "Lint code 🔍"
run: yarn eslint:check

- name: "Download webpack bundle 📦"
- name: "Build application 🏗"
trallard marked this conversation as resolved.
Show resolved Hide resolved
run: yarn run build

- name: "Build application artifacts 🏗"
trallard marked this conversation as resolved.
Show resolved Hide resolved
# ensure we use the prod target to minimise assets
run: yarn run webpack:prod bundle

- name: "Generate package tarball 📦"
run: yarn pack --filename ${{ env.PACKAGE_FILE }}

- name: "Upload package tarball 📤"
uses: actions/upload-artifact@v4
with:
name: conda-store-ui-package
path: ${{ env.PACKAGE_FILE }}

release-to-npmjs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we only want to run this job on a release but I can't seem to find anything that prevents the release-to-npm job from running on every pull request or push to the main branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have if: github.repository_owner == 'conda-incubator' && github.event_name == 'release' && startsWith(github.ref, 'refs/tags/') which serves that purpose on the attestation and release

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we want the build and verification to run at all the times and the publish to npmjs only on releases

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Doesn't this result in confusing UX in GitHub? As in, somebody creates a pull request, sees this check, clicks on it, and sees both a build and release job with green check marks, and might think, oh did my pull request trigger a release?

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 suppose that is when the logs are helpful.
I mean, I can further separate the release and have it in a completely isolated job it really makes no difference IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and we do have much better logs thanks to you. :)

But it's still bad UI, but also maybe not high priority so... just gotta live with it

name: "Release conda-store-ui to npmjs 📦"
runs-on: ubuntu-latest
needs: build-application
# needed for attestations
permissions:
id-token: write
attestations: write
contents: read

steps:
- name: "Checkout repository 🛎"
uses: actions/checkout@v4
Comment on lines +100 to +101
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this step needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes if there is no package.json then one cannot run the publish step

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, of course. But hmmm, something about the way the build and release jobs are split up still feels unintuitive to me... can't quite put my finger on it

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 do not know why this is a rather standard approach.

If you build on every PR and merge to main basically you are following continuous delivery practices -> ensures that when you need to develop and release this is is working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean the fact that they are split up, more about how... but it's not important, this can be iterated on later :)


# Setup .npmrc file to publish to npm
- name: "Set up Node.js 🧶"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this step needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes so that we have an .npmrc and can handle authentication to npmjs

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... I started playing with this, and for some reason, if you use this action with the registry-url parameter, something magical happens (something to do with .npmrc) that allows the npm publish command to pick up env.NODE_AUTH_TOKEN. Otherwise, the command doesn't pick up the token from the environment, and it fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not magically work - I am using the registry-url here and it still fails to pick up the correct credentials.

See actions/setup-node#763
which I tried and check if it was resolved, but no.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I was agreeing with you here that this step is necessary. It's just sad to me that these two things are not at all clearly related. It's side effect dependencies.

I think a comment like the following would be helpful to people later:

# Without this parameter, `npm publish` will not be able to get the API token from the environment variable `NODE_AUTH_TOKEN`
# (Why? Unsure but it seems that registry-url causes .npmrc to be saved which somehow causes npm publish to read from NODE_AUTH_TOKEN) 
registry-url: "https://registry.npmjs.org"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I don't understand why we keep linking to actions/setup-node#763. That issue is irrelevant. It's only relevant if you're using the GitHub package registry, which we're not using.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But it is not, that is what I am trying to explain.
Using the set-node action with a registry-url should configure it so that the authentication is properly setup for publication to whichever registry.
But it is not, hence I needed to add, a while back, the additional step to set the proper context.

That issue is relevant as it's the only issue where I found useful steps to properly set the context and authentication. Hence why I left it as a comment.
I tried to remove the additional context-setup again this time to see if that was fixed but the authentication was still failing, so could not remove that step. I can remove the comment linking to that issue if this is too confusing but it's helpful to me as a reference.

uses: actions/setup-node@v4
with:
node-version: "20.x"
registry-url: "https://registry.npmjs.org"
scope: "@conda-store-ui"

- name: "Download build artefacts 📦"
uses: actions/download-artifact@v4
with:
name: webpack-bundle
path: dist/
name: conda-store-ui-package

- name: "Generate package tarball 📦"
run: yarn pack --filename conda-store-ui.tgz
- name: "Attest Build Provenance ✨"
trallard marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/attest-build-provenance@v1
if: github.repository_owner == 'conda-incubator' && github.event_name == 'release' && startsWith(github.ref, 'refs/tags/')
with:
subject-path: ${{ env.PACKAGE_FILE }}

- name: "Set NPM scope" #(setup-node workaround https://github.com/actions/setup-node/issues/763)
- name: "Set npmjs scope" #(setup-node workaround https://github.com/actions/setup-node/issues/763)
env:
NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
run: |
npm config delete @conda-store-ui:registry --location project
npm config set @conda-store-ui:registry 'https://registry.npmjs.org' --location project
npm config set //registry.npmjs.org/:_authToken '${NPM_AUTH_TOKEN}' --location project
npm config set //registry.npmjs.org/:_authToken '${NODE_AUTH_TOKEN}' --location project

- name: "Check npmjs/conda-store-ui scope"
run: npm show

# we always do a dry run for the publish
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because we want to make sure the actual publish will succeed when we make a release

Copy link
Contributor

Choose a reason for hiding this comment

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

The dry run is pretty much useless unless you inspect the results. The command does not error out (exit code 1) if it encounters any issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And you either check the logs or the artefacts or both, these are not only useful based on exit codes, but logs that one can go and revisit later to diagnose.
Since we had neither to check we also did not have a quick way to check why/where the lib dir has gone missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree because the same information is going to be printed with npm publish --verbose, so this step in the context of a workflow file is 100% redundant.

I'm sorry to be pushing back so hard, but one reason I think all of this code is so hard to work with is because there's stuff in here and we have no idea why it's here or why it can't be removed. If we're going to keep this step here I think we should document in the comments why it makes sense to do the dry run immediately before the publish, because to me, it makes no sense. In other words, if I come to this workflow file and see this step before the other I will think: this makes no sense, there must be something I don't understand. But I think it just doesn't make sense. It would make sense if there were some code afterwards that grepped the console output for error messages before going to the next step, but we're not doing that here.

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 am really not privvy of either of these. I added both when refactoring this workflow so if both are confusing/useless we can just remove it.
As long as we have logs of some sort it is good enough for me.

- name: "Check publish (dry run) 📤"
run: |
echo "Publishing dry run..."
npm publish --verbose --access public ${{ env.PACKAGE_FILE }} --dry-run
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

- name: "Publish to npm 📤"
if: github.repository_owner == 'conda-incubator' && github.event_name == 'release' && startsWith(github.ref, 'refs/tags/')
run: |
echo "Publishing with tag ${{ env.GITHUB_REF_NAME }}"
npm publish --verbose --access public conda-store-ui.tgz
npm publish --verbose --access public ${{ env.PACKAGE_FILE }} --dry-run
trallard marked this conversation as resolved.
Show resolved Hide resolved
env:
NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,5 @@ static
!.yarn/sdks
!.yarn/versions
.pnp.*#

.bun.lockb
2 changes: 2 additions & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
1. Build the package locally:

```bash
# clean build artefacts to avoid issues
yarn run clean
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, the clean command doesn't currently clear the dist/ directory, and may be missing other directories that it should clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but the yarn run webpack:prod or whatever that is cleans dist.
We can do a clean:slate instead if one wants to be safe

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I don't understand why someone would need to run this command if they run already ran git clean -fxdq on line 5 above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because people not always follow steps in a linear way and mess up when running through things 🤷🏽‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true. But I would hope that someone would be a bit more careful when doing a release.

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 can remove that if it is too much cruft, as long as it makes sense to people it is fine.
And I also hope people are careful, including me, but mistakes can happen, it's just the way it is.

yarn install

# build the package
Expand Down
Binary file added bun.lockb
Binary file not shown.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,4 @@
"node": ">=18.0.0"
},
"packageManager": "[email protected]"
}
}