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

Add a 'source-root' input to the init Action #607

Merged
merged 21 commits into from
Jul 13, 2021
Merged

Add a 'source-root' input to the init Action #607

merged 21 commits into from
Jul 13, 2021

Conversation

mario-campos
Copy link
Contributor

@mario-campos mario-campos commented Jun 29, 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.

This input is exposed in the CodeQL CLI as the flag --source-root, allowing
users of the CLI to set --source-root different from --working-dir. However,
in codeql-action, these two paths are conflated and it poses problems for
users with complicated build environments, in which a source root may be
a child of the working directory.

Most users should not notice this, as the default value is
${{ github.workspace }}, as it is implied now (`path.resolve()`).
In the previous commit, the default value of the input is ${{ github.workspace }}
which means that most uses of this input would probably prefix their paths with
${{ github.workspace }}, especially since actions/checkout's 'path' input
must be under ${{ github.workspace }}. Therefore, it doesn't make much sense for
this to be an absolute file path.

Instead, it's more intuitive to make this relative to the repository.
It causes the action to break (or rather that context being unavailable causes it to fail), despite it being in the description field.
@mario-campos mario-campos requested a review from a team as a code owner June 29, 2021 21:13
init/action.yml Outdated
@@ -38,6 +38,9 @@ inputs:
description: Try to auto-install your python dependencies
required: true
default: 'true'
source-root:
description: Path to the root source-code directory, relative to the workspace.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw you had trouble using ${{ github.workspace }} in the description. In https://github.com/actions/checkout/blob/main/action.yml they use $GITHUB_WORKSPACE and it appears that

default: ${{ github.workspace }}

is also allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. That was in my first commit, but when I actually started to use it in my test, I found it inconvenient to always have to specify a ${{ github.workspace }} prefix. Hence, why I made it relative to the repo in subsequent commits.

Copy link
Collaborator

@aibaars aibaars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks!

src/init-action.ts Outdated Show resolved Hide resolved
init/action.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried there are two usages changed within the runner but only one in the init-action. It looks like there's a use of getRequiredEnvParam("GITHUB_WORKSPACE") passed to initConfig in init-action.ts that also needs changing.

Although, I'm actually not sure that changing the calls to initConfig is the right thing to do. This will change how local queries are resolved and I'm not sure if that is intended or not.

In general it depends how this option will be used. Is it for when you want to analyze the whole repository but the repository is checked out to a non-standard location? If so then this change looks good to me (minus my comment above). If instead the intention is to be able to analyze only a subdirectory of a repository, then I think this may need deeper thought to know if this will work or not.

Finally, what manual testing has been done of this? Have you tried running it in a workflow.

@robertbrignull
Copy link
Contributor

To follow on from my comment above, looking at how it works in runner.ts currently, the option is used to control where the repository being analyzed is checked out to, but we haven't done it properly. It seems wrong to me that we currently use cmd.checkoutPath in initConfig but don't use it when initializing the codeql databases. So to a large degree the changes in this PR are just fixing a bug that was present already 👍

@aibaars
Copy link
Collaborator

aibaars commented Jun 30, 2021

I'm worried there are two usages changed within the runner but only one in the init-action. It looks like there's a use of getRequiredEnvParam("GITHUB_WORKSPACE") passed to initConfig in init-action.ts that also needs changing.

Although, I'm actually not sure that changing the calls to initConfig is the right thing to do. This will change how local queries are resolved and I'm not sure if that is intended or not.

I think we should not change initConfig here. The source-root property is the equivalent of the --source-root flag of the CodeQL CLI and its only purpose it to inform CodeQL what the path to the "source files" is so it can produce the correct relative paths in the SARIF output.

@robertbrignull
Copy link
Contributor

I think we should not change initConfig here.

I've now come around to the other way of thinking. I think we should change initConfig (in the sense that we should pass the checkout path to it) because that's what we're already doing in the runnner. You can see we pass cmd.checkoutPath to initConfig and that's currently the only place we use it in the runner command.

The source-root property is the equivalent of the --source-root flag of the CodeQL CLI and its only purpose it to inform CodeQL what the path to the "source files" is so it can produce the correct relative paths in the SARIF output.

What does "path to the source files" mean? Is that the root of the github repository, or a subdirectory within a repository? When talking about relative paths in the SARIF output we need those to be relative to the root of the repository, don't we? If not then showing files in the code scanning UI will not work.

@aibaars
Copy link
Collaborator

aibaars commented Jun 30, 2021

What does "path to the source files" mean? Is that the root of the github repository, or a subdirectory within a repository? When talking about relative paths in the SARIF output we need those to be relative to the root of the repository, don't we? If not then showing files in the code scanning UI will not work.

Yes that's right. It should be the path to the root of the github repository being analysed, or perhaps more precisely the repository to which the SARIF file will be uploaded. The use-case for this feature is for complex build setups that for example require a bunch of repositories (the repo to be analysed an its dependencies) to be checked out in a particular folder hierarchy that does not match the actions/checkout's defaults .

@aibaars
Copy link
Collaborator

aibaars commented Jun 30, 2021

I've now come around to the other way of thinking. I think we should change initConfig (in the sense that we should pass the checkout path to it) because that's what we're already doing in the runnner. You can see we pass cmd.checkoutPath to initConfig and that's currently the only place we use it in the runner command.

I think there is no such thing as "the checkout path". A user can organise the workspace folder structure in any way they like either by configuring actions/checkout or even cloning repos "manually". I think a user could store the config file in another repo that is checked out somewhere in the workspace, or perhaps even have it in a fixed place on a self-hosted worker.

@mario-campos
Copy link
Contributor Author

Although, I'm actually not sure that changing the calls to initConfig is the right thing to do. This will change how local queries are resolved and I'm not sure if that is intended or not.

Yes, I deliberately did not change initConfig because I believe that when someone specifies a local query in the config file, ./ has the connotation of the root of the repository. It would be very confusing to find that it changes based on the source-root specified elsewhere in the workflow file, in my opinion.

Is it for when you want to analyze the whole repository but the repository is checked out to a non-standard location?

Yes, exactly, which, admittedly, is a minor use-case, but one that I have seen already and one which is supported in the CLI with the --source-root and --working-dir flags.

If instead the intention is to be able to analyze only a subdirectory of a repository, then I think this may need deeper thought to know if this will work or not.

My intention was not to replace paths/paths-ignore. Although, it seems it could somewhat be used to that effect.

Finally, what manual testing has been done of this? Have you tried running it in a workflow.

Yes, you can find the workflow here and the results there.

@robertbrignull
Copy link
Contributor

@mario-campos, sorry for the confusing comments above. You can likely ignore my comments and go with what @aibaars says. We had quite a long chat on this and I think we're now on the same page with this, so you can ignore anything contradictory that I said earlier.

mario-campos and others added 3 commits June 30, 2021 12:32
Previously, I had tried to refer to '${{ github.workspace }}', but that caused a problem in Actions. Trying to avoid the issue, I changed this to "the workspace," but this gives up quite a bit of specificity.

Co-authored-by: Arthur Baars <aibaars@github.com>
Thanks to @aibaars, `path.resolve()` should will nicely handle more use-cases, namely absolute paths better than `path.join()`.

Co-authored-by: Arthur Baars <aibaars@github.com>
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible. Would you be willing to add a test, here or in a follow up PR?

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

## [UNRELEASED]

- The `init` step of the Action now supports a `source-root` input to be able to specify a path under the GitHub workspace to be the root source-code directory. [#607](https://github.com/github/codeql-action/pull/607)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we mention that the default is the $GITHUB_WORKSPACE root?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mario-campos
Copy link
Contributor Author

Yes, I can add tests. Where would I do that, though? I don't see a test file for init.ts.

@adityasharad
Copy link
Contributor

Hmm good point, we don't seem to have unit tests. Perhaps another test job in pr-checks.yml, that exercises the source-root field in the init step. I think you can copy the test-packaging-javascript-config job as a template, and remove the packs inputs there. Those tests use a simple multilanguage test folder checked into the repo, so you could point explicitly to that path as the source root.

@mario-campos
Copy link
Contributor Author

Huh. Any idea why a source root would be considered invalid (aside from it not existing)?

A fatal error occurred: Invalid source root: /home/runner/work/codeql-action/codeql-action/tests/multi-language-repo.
Error: Failure invoking /opt/hostedtoolcache/CodeQL/0.0.0-20210622/x64/codeql/codeql with arguments database,init,/home/runner/work/_temp/codeql_databases/javascript,--language=javascript,--source-root=/home/runner/work/codeql-action/codeql-action/tests/multi-language-repo.

Exit code 2 and error was:

A fatal error occurred: Invalid source root: /home/runner/work/codeql-action/codeql-action/tests/multi-language-repo.

run: |
mkdir ../action
mv * .github ../action/
mv ../action/tests/multi-language-repo/{*,.github} .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is the problem: it will move all the contents of multi-language-repo into the current directory, and not preserve the tests/multi-language-repo enclosing directories.

One option to try might be to avoid these moves entirely, and set source-root as you've done below. The moves are needed for other tests because they scan the entire repo from the workspace root, but the whole point of source-root is to customise that behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. That makes sense.

mv ../action/.github/workflows .github
- uses: ./../action/init
with:
config-file: ".github/codeql/codeql-config-packaging.yml"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may be able to remove this, and keep the test focused on only the source-root feature.

.github/workflows/pr-checks.yml Outdated Show resolved Hide resolved
The test is not related to packaging.

Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com>
Moving the files into ../action was causing the job to fail because it couldn't find the test directory anymore. According to @adityasharad, these 'mv's should
not be necessary. Removing these means changing the path to the actions.

I'm also removing the 'config-file' input to keep the test minimal. I think this will mean that CodeQL will use the default query suite, so I hope that this doesn't change the results.
As this test is using only JS, it's not necessary to compile or analyze the other languages.
This is for PR #607, 'source-root' input test case.
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One test addition to actually build the database, then I think it's good to go!

.github/workflows/pr-checks.yml Show resolved Hide resolved
mario-campos and others added 2 commits July 2, 2021 14:50
Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com>
@aibaars aibaars enabled auto-merge July 13, 2021 11:13
@aibaars aibaars merged commit 3b017ef into github:main Jul 13, 2021
@mario-campos mario-campos deleted the source-root-input branch July 14, 2021 15:19
@github-actions github-actions bot mentioned this pull request Jul 19, 2021
5 tasks
aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021
aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021
aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021
aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021
aofaof0907 pushed a commit to aofaof0907/codeql-action that referenced this pull request Jul 29, 2021
This is for PR github#607, 'source-root' input test case.
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.

5 participants