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

node20 upgrade #110

Merged
merged 11 commits into from
Feb 8, 2024
Merged

node20 upgrade #110

merged 11 commits into from
Feb 8, 2024

Conversation

erikburt
Copy link
Contributor

@erikburt erikburt commented Jan 23, 2024

This is proof-of-concept work for updating this action to node20. This is in reference to #99.

I don't think this is ready to ship, although it does resolve some problems as mentioned in #99 and seen in #102.

Changes

  • Ran pnpm upgrade -L
  • Remove usage of @ts-schema-autogen/cli and replaced the run_install input validation with codified checks instead
    • Use yaml instead of js-yaml to parse the run_install input
  • Copied pnpm.cjs (and worker.js) from the pnpm release and removed existing pnpm.js file
    • Added that to dev dependencies and added a script to copy that over to the dist directory
    • I did this because I could not get it working with the included pnpm.js
  • Updated tsconfig based off @tsconfig/node20
  • Rebuilt dist/index.js

Testing

I've performed some testing, only some minimal stuff. Realistically, I haven't changed much in terms of source code. All my usages of this setup are rather simple with respect to the run_install input so I'm not the best person to test this.

  1. Tested using the run.sh with minor modifications to account for my node version shimming
run.sh contents
#! /bin/sh
# shellcheck disable=SC2155,SC2088
export HOME="$(pwd)"
export INPUT_VERSION=8
export INPUT_DEST='/tmp/pnpm-action'
export INPUT_RUN_INSTALL=true
export INPUT_STANDALONE=false
export INPUT_PACKAGE_JSON_FILE='package.json'

exec ~/.asdf/installs/nodejs/20.8.1/bin/node dist/index.js
  1. Tested using a simplistic workflow in a test repository
Workflow file
name: pull-request-main

on:
  merge_group:
  pull_request:
    branches:
      - main

jobs:

  ci-pnpm-setup-test-no-install:
    runs-on: ubuntu-latest
    steps:
      - name: Install node
        uses: actions/setup-node@v4
        with:
          node-version: 20
      - name: setup-pnpm
        uses: erikburt/action-setup@update/node20
        with:
          version: 8
          run_install: false
      - name: validate-pnpm
        shell: bash
        run: |
          pnpm --version
          pnpm install --frozen-lockfile

  ci-pnpm-setup-test-install:
    runs-on: ubuntu-latest
    steps:
      - name: Install node
        uses: actions/setup-node@v4
        with:
          node-version: 20
      - name: setup-pnpm
        uses: erikburt/action-setup@update/node20
        with:
          version: 8
          run_install: true
      - name: validate-pnpm
        shell: bash
        run: |
            pnpm --version
Output

Test 1:

Run erikburt/action-setup@update/node20
  with:
    version: 8
    run_install: false
    dest: ~/setup-pnpm
    package_json_file: package.json
    standalone: false
Running self-installer...
  Progress: resolved 1, reused 0, downloaded 0, added 0
  Packages: +1
  +
  Progress: resolved 1, reused 0, downloaded 1, added 1, done
  
  dependencies:
  + pnpm 8.14.3
  
  Done in 1s
Installation Completed!

---

Run pnpm --version
  pnpm --version
  pnpm install --frozen-lockfile
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
8.14.3
 ERR_PNPM_NO_PKG_MANIFEST  No package.json found in /home/runner/work/monorepo-test/monorepo-test
Error: Process completed with exit code 1.

Test 2:

Run erikburt/action-setup@update/node20
  with:
    version: 8
    run_install: true
    dest: ~/setup-pnpm
    package_json_file: package.json
    standalone: false
Running self-installer...
  Progress: resolved 1, reused 0, downloaded 0, added 0
  Packages: +1
  +
  Progress: resolved 1, reused 0, downloaded 1, added 1, done
  
  dependencies:
  + pnpm 8.14.3
  
  Done in 1.2s
Installation Completed!
Running pnpm recursive install...
  No projects found in "/home/runner/work/monorepo-test/monorepo-test"

---

Run pnpm --version
  pnpm --version
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
8.14.3

@erikburt erikburt mentioned this pull request Jan 23, 2024
@erikburt
Copy link
Contributor Author

erikburt commented Jan 24, 2024

Saw workflow failures for 8e68382 (9 of 27 errored). Modified to match previous behaviour of the parsing/validation method.

Now doing process.exit(1) when input is invalid instead of throwing an error, which I believe was the problem.

@zkochan
Copy link
Member

zkochan commented Jan 27, 2024

some tests fail

Copy link
Collaborator

@KSXGitHub KSXGitHub left a comment

Choose a reason for hiding this comment

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

Validating user data manually is error-prone. I suggest using zod instead.

src/inputs/run-install.ts Outdated Show resolved Hide resolved
src/inputs/run-install.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/inputs/run-install.ts Outdated Show resolved Hide resolved
src/inputs/run-install.ts Outdated Show resolved Hide resolved
@erikburt erikburt changed the title node20 upgrade proof of concept node20 upgrade Feb 5, 2024
@KSXGitHub
Copy link
Collaborator

KSXGitHub commented Feb 6, 2024

You have yet to explain why you would replace js-yaml with yaml. Does yaml have benefits that js-yaml doesn't provide?

package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@KSXGitHub KSXGitHub left a comment

Choose a reason for hiding this comment

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

@zkochan zkochan merged commit 1ee9c9d into pnpm:master Feb 8, 2024
27 checks passed
@zkochan
Copy link
Member

zkochan commented Feb 8, 2024

🚢 v3.0.0

@erikburt
Copy link
Contributor Author

erikburt commented Feb 8, 2024

Thanks for fixing my PR and merging. I can try and respond to the questions for traceability / posterity.

You have yet to explain why you would replace js-yaml with yaml. Does yaml have benefits that js-yaml doesn't provide?

I think I originally switched while trying to figure out what was broken from the 16 -> 20 upgrade. I am more familiar with yaml so I tried that.

Is this desired? (in reference to tsconfig changes)

As mentioned in the PR description I updated the tsconfig based off @tsconfig/node20. IIRC these changes were required to get it building and running properly.

@jrjohnson
Copy link

Thanks for getting this updated and released! We don't have anything complicated in our setup, but just wanted to provide at least one data point that v3 working as expected in our app.

@mCzolko
Copy link

mCzolko commented Feb 9, 2024

Great works guys, works like a charm! Thank you!

@erikburt erikburt deleted the update/node20 branch February 12, 2024 19:25
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.

7 participants