-
Notifications
You must be signed in to change notification settings - Fork 309
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
Allow local instead of downloaded CodeQL #606
Conversation
src/codeql.ts
Outdated
} | ||
let codeqlFolder: string; | ||
let codeqlURLVersion: string; | ||
if (codeqlURL?.startsWith("file://")) { |
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.
We parse this in other code paths as a URL object. Would that make sense to do here? Then you can check the scheme is file
and use the URL path to find the file, instead of string manipulation.
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.
It would be better to try to parse as a URL first, I think. file://
is not technically correct (even though it is usually accepted). file:/
and file:///
is a correct file url. But, these details could be ignored by usng new URL(codeqlURL).protocol
or something like that.
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.
Hmm, maybe let's go with @aeisenberg's suggestion below of not using file://
URLs in the first place.
34ef49a
to
5b32ed0
Compare
src/codeql.ts
Outdated
} | ||
let codeqlFolder: string; | ||
let codeqlURLVersion: string; | ||
if (codeqlURL?.startsWith("file://")) { |
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.
It would be better to try to parse as a URL first, I think. file://
is not technically correct (even though it is usually accepted). file:/
and file:///
is a correct file url. But, these details could be ignored by usng new URL(codeqlURL).protocol
or something like that.
tempDir, | ||
logger | ||
); | ||
codeqlURLVersion = "local"; |
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.
Don't know if we need to, but we could get the codeql version by runnng codeql --version
.
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.
As far as I can tell, the only use of this version number is that it goes in the status report and I think from the point of view of status reports, knowing that people are pinned to a local version of the CLI is probably more useful than having the exact version they happen to be on.
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.
Makes sense.
src/codeql.ts
Outdated
let codeqlURLVersion: string; | ||
const parsedUrl: URL | undefined = | ||
codeqlURL && codeqlURL !== "latest" ? new URL(codeqlURL) : undefined; | ||
if (parsedUrl?.protocol === "file:") { |
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.
Wonder if it would be better to just use a raw path to indicate a local file, rather than a file:
url. Technically, relative file uris are not legal, so this: file://../action/codeql-bundle.tar.gz
is invalid.
It's easy enough to distinguish a file path from an http(s) url (except for the degenerate case where someone has named a file https://thisisafile
, which I don't think we need to handle). And doing it this way will be easier might be easier for users since they don't need to remember to use a file:
protocol.
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.
Yeah, it seems reasonable to treat everything that doesn't look like a http(s) URL as a file path, let's do that.
5b32ed0
to
a7dac5c
Compare
Closes #154
This PR allows the
tools:
argument to accept an argument to specify a local CodeQL distribution instead of a remote one. This will be used internally for testing CodeQL distributions for compatibility with the Action. I do not think we need a changelog entry for this as it is not something users should be using.Reviewing note: This PR changed the indentation of a lot of a function that ended up inside of an
else
. Thus, the diff should be viewed with whitespace changes off for improved readability.Merge / deployment checklist