harden github actions by perminder-17 · Pull Request #8650 · processing/p5.js
| run: bash <(curl -s https://codecov.io/bash) -f coverage/coverage-final.json | ||
| env: | ||
| CI: true | ||
| uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5.5.2 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not really utilizing it at the moment so it probably can be skipped entirely. We'll do code coverage in Vitest for 2.x at some point and reporting can either use a service like this or even our own bot.
| env: | ||
| CI: true | ||
| - name: Run test | ||
| run: npm test -- --project=unit-tests |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I was directly using the same diff from the main branch which gets me to some unnecessary changes, Fixing that now.
| files: release/* | ||
| generate_release_notes: true | ||
| token: ${{ secrets.ACCESS_TOKEN }} | ||
| token: ${{ secrets.GITHUB_TOKEN }} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@perminder-17 I don't think the token should change (or, why should it?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Step 3 (Release p5.js), the release is created on the same repository, so GITHUB_TOKEN is sufficient. Unlike ACCESS_TOKEN, which is a long-lived Personal Access Token, GITHUB_TOKEN is automatically generated and scoped to the current repository for each workflow run, and expires once the workflow completes. ACCESS_TOKEN is only required in Step 4, where cross-repository access is needed to push changes to the p5.js-website repository. What you think?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sure! Then GITHUB_TOKEN seems alright, thank you for the explanation
| env: | ||
| CI: true | ||
| - name: Run test | ||
| - name: Run build |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (?)
| env: | ||
| CI: true | ||
| - name: Run build | ||
| run: npm run build |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also typo?
| with: | ||
| node-version: 22.x | ||
| persist-credentials: false | ||
| - name: Use Node.js 20.x |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 20?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments, thanks for working on this!
I left a few comments, thanks for working on this!
I thought the diff for dev-2.0 would be the same as main, so I directly pushed the same changes there too. But it looks like a few unnecessary changes and maybe some typos got included in the 2.0 branch. Fixing that, :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters