Replace checkoutPath variable with sourceRoot/workspacePath variables - #610
Merged
adityasharad merged 2 commits intoJul 14, 2021
Merged
adityasharad merged 2 commits into
adityasharad merged 2 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
aibaars
force-pushed
the
aibaars/refactor-checkout-path
branch
from
9fb45ac to
685dfac
Compare
July 1, 2021 13:36
Copy link
Copy Markdown
Contributor
|
Looks like a sensible renaming, thanks! That --search-path use should probably be --additional-packs, and could probably be more specifically scoped to the path of the custom query within the repo, but for now I agree with preserving it for simplicity. As we discussed internally, let's hold off on deprecating the Runner's checkout-path flag for now, since it's widely used, in favour of giving users a path towards using the CLI instead. |
Sorry, something went wrong.
aibaars
force-pushed
the
aibaars/refactor-checkout-path
branch
from
685dfac to
f94f1ed
Compare
July 14, 2021 11:39
aibaars
marked this pull request as ready for review
July 14, 2021 11:40
Copy link
Copy Markdown
Contributor
Author
|
@adityasharad shall we merge this PR? |
Sorry, something went wrong.
adityasharad
approved these changes
Jul 14, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge / deployment checklist
A discussion on #607 made me realise that the variable name checkoutPath is used for two purposes:
This pull request changes the variable name checkoutPath to be workspacePath when used for resolving paths, and sourceRoot whenever it is used as a CodeQL source-root.
There is a third use of the checkoutPath. It is passed to codeql as --search-path when running a custom query in the repository being analysed. I think this make little sense because it is very unlikely that the root folder of the repository we're analysing (or the workspace path) contains a query pack.
@adityasharad The discussion on #607 also made it clear that there is a slight inconsistency between the way the Action and the Runner handle the resolution of relative paths. The Runner resolves them with respect to the --checkout-path flag and the Action resolves them against ${GITHUB_WORKSPACE} (aka the working directory).
To make the Action and the Runner align better, I suggest we deprecate the --checkout-path flag and replace it with --source-root and --working-directory (both optional like --checkout-path and defaulting to .) . This should match the codeql cli terminology and in addition makes the Action and the Runner behave more similar.