DEV - Refactor release CI workflow #425

trallard · 2024-09-18T15:54:03Z

Description

For whatever reason, the package was not compiled during the last release. So, this PR adds additional cleanup and check tasks to the GH workflow to prevent this from happening.

Pull request checklist

Did you test this change locally?
Did you update the documentation (if required)?
Did you add/update relevant tests for this change (if required)?

Additional information

.github/workflows/build.yml

trallard · 2024-09-19T12:16:43Z

.github/workflows/javascript-tests.yml

@@ -12,7 +12,7 @@ on:
  pull_request:

 jobs:
-  build:


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

gabalafou

Left some more review comments on this latest iteration. This is shaping up nicely!

By the way, I figured out why the release for 2024.6.1 was okay, but not 9.1. It was because of my PR to make the Yarn version more explicit. Previously, the CI job was running some version of Yarn v1, after my PR it started running some version Yarn v4.

If you run yarn install with v1, it runs the package.json prepare script after installing. But for some reason that I haven't taken the time to investigate, Yarn v4 does not. The prepare script in turn calls the build script, which is what creates the crucial lib/ directory.

So when the release workflow was being run with Yarn v1, the yarn install step would also do yarn build which would result in the lib/ directory existing in the workflow runner's filesystem by the time it got to the yarn pack step.

But in my PR, which sets the Yarn version to v4, and was merged between 2024.6.1 and 2024.9.1, the yarn install step does not trigger yarn build and so the lib/ directory does not exist by the time it gets to yarn pack, which explains why the 2024.6.1 tarball in the NPM registry has a lib/ directory whereas the 2024.9.1 tarball does not.

.github/workflows/release.yml

gabalafou · 2024-09-19T13:11:16Z

.github/workflows/release.yml

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


Is this step needed?

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

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

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.

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 :)

gabalafou · 2024-09-19T13:13:21Z

.github/workflows/release.yml

+      - name: "Check npmjs/conda-store-ui scope"
+        run: npm show
+
+      #  we always do a dry run for the publish


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

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.

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.

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.

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.

.github/workflows/release.yml

gabalafou · 2024-09-19T13:13:51Z

.github/workflows/release.yml

+        uses: actions/checkout@v4
+
+      # Setup .npmrc file to publish to npm
+      - name: "Set up Node.js 🧶"


Is this step needed?

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

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.

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.

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"

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.

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.

gabalafou · 2024-09-19T13:15:01Z

RELEASE.md

@@ -7,6 +7,8 @@
 1. Build the package locally:

   ```bash
+   # clean build artefacts to avoid issues
+   yarn run clean


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

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

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.

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

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

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.

.github/workflows/release.yml

gabalafou · 2024-09-19T13:32:20Z

.github/workflows/release.yml

+          name: conda-store-ui-package
+          path: ${{ env.PACKAGE_FILE }}
+
+  release-to-npmjs:


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.

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

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

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?

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

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

Co-authored-by: gabalafou <[email protected]>

trallard · 2024-09-20T13:19:40Z

I made the last changes, which are updating the workflow step names.
That aside, this should be ready.

I still do not understand how the yarn v1/v4 thing happened/works but 🤷🏽‍♀️

gabalafou · 2024-09-20T15:22:46Z

From the NPM v10 docs on npm install:

npm install
These also run when you run npm install -g

preinstall
install
postinstall
prepublish
preprepare
prepare
postprepare

From the Yarn 1 docs on scripts:

Scripts are a great way of automating tasks related to your package, such as simple build processes or development tools. Using the "scripts" field, you can define various scripts to be run as yarn run <script>. For example, the build-project script above can be invoked with yarn run build-project and will run node build-project.js.

Certain script names are special. If defined, the preinstall script is called by yarn before your package is installed. For compatibility reasons, scripts called install, postinstall, prepublish, and prepare will all be called after your package has finished installing.

The start script value defaults to node server.js.

From the Yarn v2 docs on lifecycle scripts:

Note that we don't support every single lifecycle script originally present in npm. This is a deliberate decision based on the observation that too many lifecycle scripts make it difficult to know which one to use in which circumstances, leading to confusion and mistakes. We are open to add the missing ones on a case-by-case basis if compelling use cases are provided.

The current Yarn docs (v4) on lifecycle scripts says the same thing.

gabalafou · 2024-09-20T15:28:03Z

Actually the more relevant bit from the NPM docs is:

Life Cycle Scripts

There are some special life cycle scripts that happen only in certain situations. These scripts happen in addition to the pre<event>, post<event>, and <event> scripts.

prepare, prepublish, prepublishOnly, prepack, postpack, dependencies

prepare (since [email protected])

Runs BEFORE the package is packed, i.e. during npm publish and npm pack
Runs on local npm install without any arguments
Runs AFTER prepublish, but BEFORE prepublishOnly
NOTE: If a package being installed through git contains a prepare script, its dependencies and devDependencies will be installed, and the prepare script will be run, before the package is packaged and installed.
As of npm@7 these scripts run in the background. To see the output, run with: --foreground-scripts.

gabalafou

I started reviewing this again last night and had some comments. However, I think this is fine to merge now and we can iterate on it in subsequent PRs.

gabalafou · 2024-09-19T14:51:02Z

.github/workflows/release.yml

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


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

gabalafou · 2024-09-19T14:54:20Z

.github/workflows/release.yml

+        uses: actions/checkout@v4
+
+      # Setup .npmrc file to publish to npm
+      - name: "Set up Node.js 🧶"


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.

gabalafou · 2024-09-19T14:59:09Z

RELEASE.md

@@ -7,6 +7,8 @@
 1. Build the package locally:

   ```bash
+   # clean build artefacts to avoid issues
+   yarn run clean


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.

gabalafou · 2024-09-19T22:04:13Z

.github/workflows/release.yml

+      - name: "Check npmjs/conda-store-ui scope"
+        run: npm show
+
+      #  we always do a dry run for the publish


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.

gabalafou · 2024-09-19T22:10:45Z

.github/workflows/release.yml

+          name: conda-store-ui-package
+          path: ${{ env.PACKAGE_FILE }}
+
+  release-to-npmjs:


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?

gabalafou

I started reviewing this again last night and had some comments. However, I think this is fine to merge now and we can iterate on it in subsequent PRs.

gabalafou

Oh wait you need to get rid of the bun.lockb file first

… trallard/update-release-wf

trallard · 2024-09-23T14:52:17Z

The last push addresses all of @gabalafou concerns and any improvements will have to come later as we only needed to get a working pipeline to try and make a pre-release and this has been hanging for too long now

trallard added 2 commits September 18, 2024 16:50

Add clean step to release

67e703a

Add dry-run to verify packing

fca48c0

trallard added the area: CI 👷🏽‍♀️ label Sep 18, 2024

trallard added 4 commits September 18, 2024 17:25

Update artefact download and publish

d3625e2

Remove extra run

90716b6

Add target for build - push yaml file

4760961

Fix event trigger

a9f62d0

gabalafou reviewed Sep 19, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

trallard added 7 commits September 19, 2024 11:10

Consolidate build and release

493f8ce

Try and debug npm auth

2dcbdad

Add config back again

d2f53e1

Add debugging to current dir

4ae2946

Checkout repo for dry-run

902fc9c

Change download path

fc026d7

Remove debugging strings

2b8db4e

trallard changed the title ~~DEV - Ensure package is always compiled~~ DEV - Refactor release CI workflow Sep 19, 2024

trallard commented Sep 19, 2024

View reviewed changes

gabalafou reviewed Sep 19, 2024

View reviewed changes

trallard and others added 2 commits September 19, 2024 15:10

Apply suggestions from code review

a3f7b51

Co-authored-by: gabalafou <[email protected]>

Update workflow names

39bcb6e

Merge branch 'main' into trallard/update-release-wf

7adaabb

gabalafou approved these changes Sep 20, 2024

View reviewed changes

gabalafou requested changes Sep 20, 2024

View reviewed changes

trallard added 2 commits September 23, 2024 15:04

Merge remote-tracking branch 'origin/trallard/update-release-wf' into…

42b9628

… trallard/update-release-wf

Split verify and publish before merge

3ca80b5

gabalafou approved these changes Sep 23, 2024

View reviewed changes

trallard merged commit e611866 into main Sep 23, 2024
7 checks passed

trallard deleted the trallard/update-release-wf branch September 23, 2024 15:01

DEV - Refactor release CI workflow #425

DEV - Refactor release CI workflow #425

Conversation

trallard commented Sep 18, 2024

Description

Pull request checklist

Additional information

Choose a reason for hiding this comment

gabalafou left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trallard commented Sep 20, 2024

gabalafou commented Sep 20, 2024

From the NPM v10 docs on npm install:

From the Yarn 1 docs on scripts:

From the Yarn v2 docs on lifecycle scripts:

gabalafou commented Sep 20, 2024 • edited Loading

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

trallard commented Sep 23, 2024

gabalafou left a comment •

edited

Loading

gabalafou commented Sep 20, 2024 •

edited

Loading