pnpm / action-setup

Install pnpm package manager
https://github.com/marketplace/actions/setup-pnpm
MIT License
939 stars 89 forks source link

node20 upgrade #110

Closed erikburt closed 9 months ago

erikburt commented 10 months ago

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

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 ```shell #! /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 ```yaml 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 commented 9 months ago

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 commented 9 months ago

some tests fail

KSXGitHub commented 9 months ago

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

zkochan commented 9 months ago

:ship: v3.0.0

erikburt commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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