Remove toolcache decorator by henrymercer · Pull Request #1104 · github/codeql-action · GitHub

This decorator enabled us to use the functionality of the Actions toolcache within the runner too. Now that we no longer release the runner we no longer need it.

Merge / deployment checklist

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

This decorator enabled us to use the functionality of the Actions toolcache within the runner too. Now that we've deleted the runner we no longer need it.

Rebased to fix a merge conflict in one of the sourcemap files.

I sometimes run the runner locally in order to test out changes before pushing them. Does this PR essentially break the runner? (Even if so, it's not a reason not to merge this change, but it's just something I'd like to be aware of.

minor: We should probably just move rm -rf lib into the npm build script itself.

Hmm, wouldn't that break incremental compilation though?

Incremental compilation is done through tsc --watch. At least that's how I run it. tsc --build is always a full build.

My understanding is tsc --build does some incremental stuff to speed itself up: https://www.typescriptlang.org/docs/handbook/project-references.html#build-mode-for-typescript. It's unfortunate tsc doesn't have support for deleting unneeded js/jsmap files, but I'd rather keep the distinction between a build and a clean build. Personally I chain npm run build into other commands and end up running it fairly frequently, but only delete TypeScript files rarely.

Does this PR essentially break the runner?

To an extent, yes. All the PR checks that involve the runner continue to work, but the runner can no longer manage its own CodeQL bundle.

This would also be a good time to uninstall any unused dependencies.

Are these still being used?

import * as toolrunner from "@actions/exec/lib/toolrunner";
import * as io from "@actions/io";
import * as actionsToolcache from "@actions/tool-cache";
import * as safeWhich from "@chrisgavin/safe-which";
import del from "del";
import * as semver from "semver";
import { v4 as uuidV4 } from "uuid";

If not, then let's remove them from the node_modules. I guess it can be in a future PR, but let's not forget them.

This would also be a good time to uninstall any unused dependencies.

Good idea. I checked and these dependencies are all still being used.

Minor comment. This can be merged without making the change, especially if there is a reason for doing this that I'm not seeing.

Was this header not being set previously? Is there any reason why you are creating a new object here and not setting "User-Agent" when you initially create the headers object?

eg-

        const headers: OutgoingHttpHeaders = {
          accept: "application/octet-stream",
          "User-Agent": "CodeQL Action"
        };

This is just existing code being moved — we can take this as an opportunity to revisit it though. Since this PR has been open for a while, I'll merge without making the change since I'd like to get a confirmation that the toolcache isn't expecting this particular user agent.

henrymercer force-pushed the henrymercer/remove-toolcache-decorator branch from a10c3a0 to ae0f195 Compare June 28, 2022 16:32

henrymercer marked this pull request as ready for review June 28, 2022 17:12

henrymercer requested a review from a team as a code owner June 28, 2022 17:12

henrymercer force-pushed the henrymercer/remove-toolcache-decorator branch 2 times, most recently from eaee63c to 41d6ac4 Compare June 28, 2022 17:22

henrymercer mentioned this pull request Jun 29, 2022

Run unit tests on Windows too #1099

Merged

3 tasks

henrymercer added 2 commits June 30, 2022 09:16

Remove toolcache decorator

c736697

This decorator enabled us to use the functionality of the Actions toolcache within the runner too. Now that we've deleted the runner we no longer need it.

Add instructions to remove deleted JS / source map files in PR check

b1742f8

henrymercer force-pushed the henrymercer/remove-toolcache-decorator branch from 9953936 to b1742f8 Compare June 30, 2022 08:16

aeisenberg reviewed Jun 30, 2022

View reviewed changes

henrymercer requested a review from aeisenberg July 1, 2022 17:24

aeisenberg reviewed Jul 11, 2022

View reviewed changes

aeisenberg approved these changes Jul 11, 2022

View reviewed changes

henrymercer merged commit ea8fb21 into main Jul 11, 2022

henrymercer deleted the henrymercer/remove-toolcache-decorator branch July 11, 2022 16:54

This was referenced Jul 13, 2022

Merge main into releases/v2 #1140

Merged

Merge releases/v2 into releases/v1 #1142

Merged

               if [ ! -z "$(git status --porcelain)" ]; then
                   # If we get a fail here then the PR needs attention
-                  >&2 echo "Failed: JavaScript files are not up to date. Run 'npm run-script build' to update"
+                  >&2 echo "Failed: JavaScript files are not up to date. Run 'rm -rf lib && npm run-script build' to update"

+                      const finalHeaders = Object.assign(
+                        { "User-Agent": "CodeQL Action" },
+                        headers
+                      );

Remove toolcache decorator by henrymercer · Pull Request #1104 · github/codeql-action · GitHub

Remove toolcache decorator - #1104

Remove toolcache decorator#1104
henrymercer merged 2 commits into
mainfrom
henrymercer/remove-toolcache-decorator

henrymercer commented Jun 17, 2022

Uh oh!

henrymercer commented Jun 30, 2022

Uh oh!

aeisenberg left a comment

Uh oh!

aeisenberg Jun 30, 2022

Uh oh!

henrymercer Jun 30, 2022

Uh oh!

aeisenberg Jun 30, 2022

Uh oh!

henrymercer Jun 30, 2022

Uh oh!

henrymercer commented Jun 30, 2022

Uh oh!

aeisenberg left a comment •
edited
Loading

Uh oh!

Uh oh!

henrymercer commented Jul 1, 2022

Uh oh!

aeisenberg left a comment

Uh oh!

aeisenberg Jul 11, 2022

Uh oh!

henrymercer Jul 11, 2022 •
edited
Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove toolcache decorator by henrymercer · Pull Request #1104 · github/codeql-action · GitHub

Remove toolcache decorator - #1104

Remove toolcache decorator#1104henrymercer merged 2 commits intomaingithub/codeql-action:mainfrom henrymercer/remove-toolcache-decoratorgithub/codeql-action:henrymercer/remove-toolcache-decoratorCopy head branch name to clipboard

Conversation

henrymercer commented Jun 17, 2022

Merge / deployment checklist

Uh oh!

henrymercer commented Jun 30, 2022

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

henrymercer Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

aeisenberg Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

henrymercer Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

henrymercer commented Jun 30, 2022

Uh oh!

aeisenberg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer commented Jul 1, 2022

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

henrymercer Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove toolcache decorator#1104
henrymercer merged 2 commits into
mainfrom
henrymercer/remove-toolcache-decorator

aeisenberg left a comment •
edited
Loading

henrymercer Jul 11, 2022 •
edited
Loading