Skip to content
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

Replace checkoutPath variable with sourceRoot/workspacePath variables #610

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

aibaars
Copy link
Collaborator

@aibaars aibaars commented Jul 1, 2021

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.

A discussion on #607 made me realise that the variable name checkoutPath is used for two purposes:

  • resolving relative file paths (config file and queries)
  • the CodeQL source root for making relative paths in the SARIF output

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.

@aibaars aibaars force-pushed the aibaars/refactor-checkout-path branch from 9fb45ac to 685dfac Compare July 1, 2021 13:36
@adityasharad
Copy link
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.

@aibaars aibaars force-pushed the aibaars/refactor-checkout-path branch from 685dfac to f94f1ed Compare July 14, 2021 11:39
@aibaars aibaars marked this pull request as ready for review July 14, 2021 11:40
@aibaars aibaars requested a review from a team as a code owner July 14, 2021 11:40
@aibaars
Copy link
Collaborator Author

aibaars commented Jul 14, 2021

@adityasharad shall we merge this PR?

@adityasharad adityasharad merged commit 14deaf6 into main Jul 14, 2021
@adityasharad adityasharad deleted the aibaars/refactor-checkout-path branch July 14, 2021 15:14
@github-actions github-actions bot mentioned this pull request Jul 19, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants