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 all 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
135 changes: 109 additions & 26 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
@@ -1,58 +1,141 @@
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 source code (library) 🏗"
run: yarn run build

- name: "Build source code (web app) 🏗"
# 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 }}

verify-build:
name: "Verify conda-store-ui build"
runs-on: ubuntu-latest
needs: build-application

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

- 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
# (setup-node workaround https://github.com/actions/setup-node/issues/763) otherwise the authentication fails for npmjs
- name: "Set npmjs scope"
env:
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 '${NODE_AUTH_TOKEN}' --location project

- name: "Set NPM scope" #(setup-node workaround https://github.com/actions/setup-node/issues/763)
- name: "Check publish (dry run) 📤"
run: |
echo "Publishing dry run..."
npm publish --verbose --access public ${{ env.PACKAGE_FILE }} --dry-run
env:
NPM_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

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
- verify-build
if: github.repository_owner == 'conda-incubator' && github.event_name == 'release' && startsWith(github.ref, 'refs/tags/')
# 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 :)


# Set registry in .npmrc and set up auth to read in from
# env.NODE_AUTH_TOKEN.
- 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: conda-store-ui-package

# Create an attestation with GitHub to track build provenance
# More info: https://docs.github.com/en/actions/security-for-github-actions/using-artifact-attestations/using-artifact-attestations-to-establish-provenance-for-builds
- 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 }}

# (setup-node workaround https://github.com/actions/setup-node/issues/763) otherwise the authentication fails for npmjs
- name: "Set npmjs scope"
env:
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

- name: "Publish to npm 📤"
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 }}
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
4 changes: 2 additions & 2 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
```

> [!IMPORTANT]
> You need to be logged in to the npmjs registry to publish the package.
> And have access to the conda-store npm namespace.
> You need to be logged in to the `npmjs` registry to publish the package.
> And have access to the `conda-store` npm namespace.

1. Perform a local dry run publish:

Expand Down
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]"
}
}