codeql-action icon indicating copy to clipboard operation
codeql-action copied to clipboard

Remove node_modules and compile typescript into minified scripts

Open jsoref opened this issue 1 year ago • 7 comments

  • Bundle action using esbuild -- the main change - fixes #2542
    • deleted lib and node_modules
    • changed */action.yml to use local minified bundled files
    • minified+bundled: */*action.js + */*action-post.js
    • added package.sh which is responsible for regenerating the minified files and fixing up the action files.

Merge / deployment checklist

  • [ ] Confirm this change is backwards compatible with existing workflows.
  • [x] Confirm the readme has been updated if necessary.
  • [x] Confirm the changelog has been updated if necessary.

jsoref avatar Nov 05 '24 02:11 jsoref

Before github/codeql-action@v3 662472033e021d55d94146f66f6058822b0b39fd (6s)

Fri, 01 Nov 2024 13:42:39 GMT Prepare all required actions
Fri, 01 Nov 2024 13:42:39 GMT Getting action download info
Fri, 01 Nov 2024 13:42:39 GMT Download action repository 'github/codeql-action@v3' (SHA:662472033e021d55d94146f66f6058822b0b39fd)
Fri, 01 Nov 2024 13:42:44 GMT Run ./.git/check-spelling-actions/upload-sarif
Fri, 01 Nov 2024 13:42:44 GMT Run github/codeql-action/upload-sarif@v3
Fri, 01 Nov 2024 13:42:44 GMT Uploading results
Fri, 01 Nov 2024 13:42:44 GMT   Processing sarif files: ["/tmp/tmp.yvZ4B9jZAF"]
Fri, 01 Nov 2024 13:42:44 GMT   Validating /tmp/tmp.yvZ4B9jZAF
Fri, 01 Nov 2024 13:42:44 GMT   Combining SARIF files using the CodeQL CLI
Fri, 01 Nov 2024 13:42:44 GMT   Adding fingerprints to SARIF file. See https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs for more information.
Fri, 01 Nov 2024 13:42:44 GMT   Uploading results
Fri, 01 Nov 2024 13:42:45 GMT   Successfully uploaded results

After check-spelling-sandbox/github-codeql-action@thin 3dc118c13c28b2804932fce08b479e77e5007079 (2s)

Tue, 05 Nov 2024 13:22:18 GMT Prepare all required actions
Tue, 05 Nov 2024 13:22:18 GMT Getting action download info
Tue, 05 Nov 2024 13:22:18 GMT Download action repository 'check-spelling-sandbox/github-codeql-action@thin' (SHA:3dc118c13c28b2804932fce08b479e77e5007079)
Tue, 05 Nov 2024 13:22:19 GMT Run ./.git/check-spelling-actions/upload-sarif
Tue, 05 Nov 2024 13:22:19 GMT Run check-spelling-sandbox/github-codeql-action/upload-sarif@thin
Tue, 05 Nov 2024 13:22:20 GMT Uploading results
Tue, 05 Nov 2024 13:22:20 GMT   Processing sarif files: ["/tmp/tmp.CoFUHRgCYx"]
Tue, 05 Nov 2024 13:22:20 GMT   Validating /tmp/tmp.CoFUHRgCYx
Tue, 05 Nov 2024 13:22:20 GMT   Combining SARIF files using the CodeQL CLI
Tue, 05 Nov 2024 13:22:20 GMT   Adding fingerprints to SARIF file. See https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs for more information.
Tue, 05 Nov 2024 13:22:20 GMT   Uploading results
Tue, 05 Nov 2024 13:22:20 GMT   Successfully uploaded results

jsoref avatar Nov 05 '24 13:11 jsoref

Thanks for your contribution. I appreciate the work you've put into this change. Before I can have a deeper look, there are some things I'd like to see:

  1. Please separate the spelling and other minor changes into a different PR.
  2. Can you explain why you need to add the upload/download artifact shims? We are not running these tests on GHES. We probably do need to update the remaining references to upload-artifact@v3, but I see no reason we need both.
  3. How does this change the development process? Do we need to run npm i && npm build before developing locally? CONTRIBUTING.md will need to be updated (but this can happen after we do a deeper review).

aeisenberg avatar Nov 06 '24 03:11 aeisenberg

  1. Please separate the spelling and other minor changes into a different PR.
    • Sure, I've spun up a check pass for this, but moved to #2580 (note that it will have conflicts in one compiled action as MacOS is present in it)
  2. Can you explain why you need to add the upload/download artifact shims? We are not running these tests on GHES. We probably do need to update the remaining references to upload-artifact@v3, but I see no reason we need both.
    • The warnings/having v3 stuff come in were annoying and it looked like it mostly shouldn't be there. The idea that one can't properly upgrade a package so that everyone uses a single version of an action is a problem. I release an action that uses this action, and it's pretty crazy for me to have to think about actions that I use and potentially pull in two different versions just so that my action won't break if it's used in GHES (I don't have access to GHES, so I really don't know if it will break or not). -- Spirit moved to https://github.com/github/codeql-action/pull/2583
  3. How does this change the development process? Do we need to run npm i && npm build before developing locally? CONTRIBUTING.md will need to be updated (but this can happen after we do a deeper review).
    • There's a paragraph about not running npm install. The text about node_modules will need to be removed. I'm not actually sure about the general macOS bits. I built this on macOS and there's no sign of fsevents, so it's possible that the macOS only thing will no longer apply.
    • Yes, the text about not running npm install will need to change. I think it should generally suggest npm ci && npm run build instead of npm run build
    • Initial changes

jsoref avatar Nov 06 '24 06:11 jsoref

That second item is mostly to deal with annoying things like: https://github.com/check-spelling-sandbox/github-codeql-action/actions/runs/11717020444

Export file baseline information (windows-latest, nightly-latest) The following actions use a deprecated Node.js version and will be forced to run on node20: actions/upload-artifact@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/ Export file baseline information (macos-latest, nightly-latest) The following actions use a deprecated Node.js version and will be forced to run on node20: actions/upload-artifact@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/ Export file baseline information (ubuntu-latest, nightly-latest) An error occurred when trying to refresh keys from hkp://keyserver.ubuntu.com: Error: The process '/usr/bin/gpg' failed with exit code 2 Export file baseline information (ubuntu-latest, nightly-latest) An error occurred when trying to refresh keys from hkp://keyserver.ubuntu.com: Error: The process '/usr/bin/gpg' failed with exit code 2 Deprecation notice: v1, v2, and v3 of the artifact actions The following artifacts were uploaded using a version of actions/upload-artifact that is scheduled for deprecation: "with-baseline-information-macos-latest-nightly-latest.sarif.json", "with-baseline-information-windows-latest-nightly-latest.sarif.json". Please update your workflow to use v4 of the artifact actions. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Of which there were a lot. Anyway, the commit is currently nowhere. I can make a PR for it, or a simpler one that just upgrades. But the current state is https://github.com/github/codeql-action/pull/2046#issuecomment-1861579420. I think someone (me?) may have merged and then had the upgrade reverted when someone realized it broke GHES, but it's too late in my day to find that item...

(In developing this PR, my goal was 0 errors and 0 warnings before opening it, which took a bit of poking.)

jsoref avatar Nov 07 '24 06:11 jsoref

After you address the merge conflicts then I can run the tests again.

Also, in a separate PR, you can upgrade all of the references to *-artifact@v3 to @v4 in our workflows. These will need to change before @v3 is removed.

aeisenberg avatar Nov 07 '24 23:11 aeisenberg

I hadn't thought through .github/workflows/update-dependencies.yml

It's referenced by: https://github.com/github/codeql-action/blob/3ef4c0845750690942ece9abe29a853edce0f43c/.github/workflows/script/check-node-modules.sh#L16

https://github.com/github/codeql-action/blob/3ef4c0845750690942ece9abe29a853edce0f43c/.github/update-release-branch.py#L100-L101

https://github.com/github/codeql-action/blob/3ef4c0845750690942ece9abe29a853edce0f43c/.github/dependabot.yml#L9-L10

https://github.com/github/codeql-action/blob/3ef4c0845750690942ece9abe29a853edce0f43c/.github/workflows/post-release-mergeback.yml#L136-L137

https://github.com/github/codeql-action/blob/3ef4c0845750690942ece9abe29a853edce0f43c/.github/workflows/post-release-mergeback.yml#L156-L163

Thinking about what they're doing also gave me a headache. In the old world, updating dependencies involved committing all of the files in node_modules (and maybe committing lib??). In the new world, if you update package.json/package-lock.json then you need to update all of the actions (as managed by package.sh) which means someone somewhere still needs to do that.

For dependabot, that appeared as https://github.com/github/codeql-action/pull/2575#issuecomment-2455312166

I think that the same stuff still needs to exist, in that we would want a commit (for at least dependabot) that updates the bundled files.

I think what we want to do is to keep the workflow but change its work from committing the node_modules to committing the bundles see https://github.com/github/codeql-action/pull/2578/commits/f73e1b66a2dae6031252e5daa459a533ea2ea654.

jsoref avatar Nov 08 '24 00:11 jsoref

I've updated .gitattributes.

jsoref avatar Nov 08 '24 00:11 jsoref

Thanks for your work on this. We've now fixed this issue in a different way. See https://github.com/github/codeql-action/pull/3054. Closing this PR now.

aeisenberg avatar Aug 29 '25 16:08 aeisenberg