-
Notifications
You must be signed in to change notification settings - Fork 6
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
upgrade to node v18, GHA #32
base: master
Are you sure you want to change the base?
Conversation
d8a8bbb
to
0b36ae0
Compare
.github/workflows/ci.yml
Outdated
with: | ||
node-version-file: .nvmrc | ||
- name: Install dependencies | ||
run: npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm ci
is more appropriate for CI jobs
run: npm install | |
run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, was just testing stuff. will eventually change it to ci
.github/workflows/ci.yml
Outdated
- name: Build Sample | ||
run: npm run build-sample | ||
- name: Report Coverage | ||
run: npm run report-coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: needs empty line
.github/workflows/release.yml
Outdated
DEFAULT_BRANCH: master | ||
GITHUB_TOKEN: ${{ secrets.D2L_GITHUB_TOKEN }} | ||
NPM: true | ||
NPM_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: needs empty line
.nvmrc
Outdated
@@ -0,0 +1 @@ | |||
18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: needs empty line
.gitignore
Outdated
@@ -3,3 +3,4 @@ coverage | |||
sample/dist | |||
node_modules | |||
npm-debug.log | |||
package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you excluding package-lock.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, I made this PR in this repo from #31 as it being a PR from forked repo, it was not triggering Actions. First i wanted to test this part whether the Actions are being triggered and if the pipeline is successful.
Even i was not sure why was package-log excluded. Will check that and remove this change.
@@ -1,19 +0,0 @@ | |||
language: node_js | |||
node_js: | |||
- 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old CI was using Node 10, while the new one is at 18. This is likely the cause of failures. You'll likely need to update dependencies for Node 18 to work.
No description provided.