Skip to content

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

Merged
AllanGuigou merged 4 commits intomainfrom
bump-node-to-v18
Nov 29, 2023
Merged

Update base image to node:20.10-buster-slim#287
AllanGuigou merged 4 commits intomainfrom
bump-node-to-v18

Conversation

@AllanGuigou
Copy link
Copy Markdown
Contributor

@AllanGuigou AllanGuigou commented Nov 28, 2023

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

@AllanGuigou
Update base image to node:18.18.2-buster-slim
c23c7ec 
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
Copy Markdown
Contributor

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!

AllanGuigou
AllanGuigou commented Nov 28, 2023
View reviewed changes
package.json
"@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
Copy Markdown
Contributor Author

@AllanGuigou AllanGuigou Nov 28, 2023 •
edited
Loading

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
Copy Markdown
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.

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

Copy link
Copy Markdown
Member

@rentziass rentziass Nov 29, 2023

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
Copy Markdown
Contributor Author

AllanGuigou 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!

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

rentziass
rentziass reviewed Nov 29, 2023
View reviewed changes
@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
rentziass
rentziass approved these changes Nov 29, 2023
View reviewed changes
Copy link
Copy Markdown
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
@AllanGuigou AllanGuigou mentioned this pull request Nov 29, 2023
Merged
@pandaapo pandaapo mentioned this pull request Nov 29, 2023
Closed
3 tasks
@tuhaihe
Copy link
Copy Markdown

tuhaihe commented Nov 30, 2023

It works, thanks!

@tuhaihe tuhaihe mentioned this pull request Nov 30, 2023
Closed
9 tasks
@thewtex thewtex mentioned this pull request Nov 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rentziass rentziass rentziass approved these changes

+2 more reviewers

@Letsgoque Letsgoque Letsgoque approved these changes

@max09000-tec max09000-tec max09000-tec approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@AllanGuigou @pje @tuhaihe @rentziass @Letsgoque @max09000-tec