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

Update base image to node:20.10-buster-slim #287

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Conversation

AllanGuigou
Copy link
Contributor

Add dependency on @octokit/rest to satisfy build requirements. Without @octokit/rest we would need to install @octokit/core, @octokit/plugin-paginate-rest and @octokit/plugin-rest-endpoint-methods.

This should resolve #286

Add dependency on @octokit/rest to satisfy build requirements.
@AllanGuigou AllanGuigou requested a review from a team as a code owner November 28, 2023 20:48
@pje
Copy link
Member

pje commented Nov 28, 2023

Seems like we have a build issue after the node upgrade:

internal/modules/cjs/loader.js:1032
  throw err;
  ^

Error: Cannot find module 'node:assert'
Require stack:
- /node_modules/undici/lib/handler/RetryHandler.js
- /node_modules/undici/index.js
- /node_modules/@actions/http-client/lib/index.js
- /node_modules/@actions/core/lib/oidc-utils.js
- /node_modules/@actions/core/lib/core.js
- /lib/main.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1029:15)
    at Function.Module._load (internal/modules/cjs/loader.js:898:27)
    at Module.require (internal/modules/cjs/loader.js:1089:19)
    at require (internal/modules/cjs/helpers.js:73:18)
    at Object.<anonymous> (/node_modules/undici/lib/handler/RetryHandler.js:1:[16](https://github.com/actions/first-interaction/actions/runs/7024356281/job/19112904850?pr=287#step:4:17))
    at Module._compile (internal/modules/cjs/loader.js:1200:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
    at Module.load (internal/modules/cjs/loader.js:1049:32)
    at Function.Module._load (internal/modules/cjs/loader.js:937:14)
    at Module.require (internal/modules/cjs/loader.js:1089:[19](https://github.com/actions/first-interaction/actions/runs/7024356281/job/19112904850?pr=287#step:4:20)) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/node_modules/undici/lib/handler/RetryHandler.js',
    '/node_modules/undici/index.js',
    '/node_modules/@actions/http-client/lib/index.js',
    '/node_modules/@actions/core/lib/oidc-utils.js',
    '/node_modules/@actions/core/lib/core.js',
    '/lib/main.js'
  ]
}

Otherwise this makes sense and I'm happy to 👍 when the build's passing!

@@ -29,8 +29,10 @@
"@actions/core": "file:toolkit/actions-core-1.10.0.tgz",
"@actions/exec": "file:toolkit/actions-exec-1.1.1.tgz",
"@actions/github": "file:toolkit/actions-github-5.1.1.tgz",
"@actions/http-client": "^2.2.0",
Copy link
Contributor Author

@AllanGuigou AllanGuigou Nov 28, 2023

Choose a reason for hiding this comment

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

i wasn't able to use the local tarball like the rest of the depencies for http-client 😕

Error: Cannot find module '@actions/http-client'

is that pattern necessary to follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also had to update the license manually from other to mit -> .licenses/npm/@actions/http-client.dep.yml

Copy link
Member

Choose a reason for hiding this comment

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

The license issue is due to http-client LICENSE being oddly formatted, we should probably fix that.

@AllanGuigou
Copy link
Contributor Author

Seems like we have a build issue after the node upgrade:

internal/modules/cjs/loader.js:1032
  throw err;
  ^

Error: Cannot find module 'node:assert'
Require stack:
- /node_modules/undici/lib/handler/RetryHandler.js
- /node_modules/undici/index.js
- /node_modules/@actions/http-client/lib/index.js
- /node_modules/@actions/core/lib/oidc-utils.js
- /node_modules/@actions/core/lib/core.js
- /lib/main.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1029:15)
    at Function.Module._load (internal/modules/cjs/loader.js:898:27)
    at Module.require (internal/modules/cjs/loader.js:1089:19)
    at require (internal/modules/cjs/helpers.js:73:18)
    at Object.<anonymous> (/node_modules/undici/lib/handler/RetryHandler.js:1:[16](https://github.com/actions/first-interaction/actions/runs/7024356281/job/19112904850?pr=287#step:4:17))
    at Module._compile (internal/modules/cjs/loader.js:1200:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
    at Module.load (internal/modules/cjs/loader.js:1049:32)
    at Function.Module._load (internal/modules/cjs/loader.js:937:14)
    at Module.require (internal/modules/cjs/loader.js:1089:[19](https://github.com/actions/first-interaction/actions/runs/7024356281/job/19112904850?pr=287#step:4:20)) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/node_modules/undici/lib/handler/RetryHandler.js',
    '/node_modules/undici/index.js',
    '/node_modules/@actions/http-client/lib/index.js',
    '/node_modules/@actions/core/lib/oidc-utils.js',
    '/node_modules/@actions/core/lib/core.js',
    '/lib/main.js'
  ]
}

Otherwise this makes sense and I'm happy to 👍 when the build's passing!

@pje was that from https://github.com/actions/first-interaction/actions/runs/7024356281? I believe that is the current first-interaction action version 😄

Dockerfile Outdated Show resolved Hide resolved
@AllanGuigou AllanGuigou changed the title Update base image to node:18.18.2-buster-slim Update base image to node:20.10-buster-slim Nov 29, 2023
Copy link
Member

@rentziass rentziass left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@AllanGuigou AllanGuigou merged commit 001cc8d into main Nov 29, 2023
3 checks passed
@tuhaihe
Copy link

tuhaihe commented Nov 30, 2023

It works, thanks!

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.

It's been 2 days since the action is failing (used to work fine)
4 participants