-
Notifications
You must be signed in to change notification settings - Fork 429
Allow the codeql-action to be run locally #117
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
Conversation
src/util.ts
Outdated
| } | ||
|
|
||
| core.debug('Action is running locally.'); | ||
| if (!process.env.RUNNER_TEMP) { |
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.
I'm a bit surprise that this line is needed. See also https://github.com/nektos/act/blob/64b8d2afa47ff6938d7617e00f1260a95c35268e/pkg/runner/run_context.go#L82
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.
When I run a workflow containing the following with act
- run: |
printenv
I see that RUNNER_TEMP=/tmp.
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.
Hmmmm. Again, this was not the case for me. I must be running things differently somehow or a different version of act. I'll have to explore.
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.
Ha! Upgraded from 0.2.7 -> 0.2.10 and that environment variable is now being set. Knowing this yesterday would have made things go a lot more smoothly.
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.
Thanks for catching that.
|
Setting variables in a file named |
|
Hmmm...The .env did not work for me. I wonder if I'm using an older version than you. Also, are you on Linux? |
Yes, on linux. |
|
When generating the status reports, we access the github API to get the workflow ID. This would also need to be dealt with somehow. Probably best to audit all uses of the |
|
@robertbrignull I believe that is already handled here: https://github.com/github/codeql-action/pull/117/files#diff-8cd4968b81985f0efc2053eabc37db6dR263 Under local mode, no status messages are sent. There is at least one call to |
That stops the reports from being sent, but the call I was thinking of is when we are constructing the data to send, which happens before that. However now I look closely it is already covered by https://github.com/github/codeql-action/pull/117/files#diff-8cd4968b81985f0efc2053eabc37db6dR102 as this is the method I was thinking of.
I haven't used |
|
Yes, that is what I was proposing indirectly above. I could do something like this: export const getApiClient = function(allowLocalRun = false) {
if (isLocalRun() && !allowLocalRun) {
throw new Error('Invalid API call in local run');
}
...And then for calls we know to be safe, we pass the |
3966be4 to
cd5c2ae
Compare
|
I'm confused why I'm getting errors on the Code scanning action. This is coming from code that I don't think I have changed. It looks like this check was not run for #116, though it was run for #115 (and it failed, too, but for different checks). Is it possible the |
|
Even more confusing is that this failing check is not showing up in the actions tab, but previous calls to the codeql actions were invoked on this branch and were successful. Eg- https://github.com/github/codeql-action/actions/runs/176329168 |
ac386f7 to
ed210a5
Compare
5f292ee to
b0ece22
Compare
|
The Code Scanning check is the one that reports changes in alerts. So when it is marked as failed it's because that PR introduces alerts. Things might be a little confused right now because we had to revert some changes to the |
|
Thanks for the clarification. |
|
@robertbrignull is this a worthwhile thing to add to the action? |
robertbrignull
left a comment
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.
Sorry this languished slightly. I wasn't sure if @aibaars was going to carry on reviewing.
I've made a couple of extra comments and then I'd be happy for this to go in. It's probably not a feature that I will use myself when testing the action, but I can see that it would be very useful for codeql.
src/util.ts
Outdated
| /** | ||
| * Ensures all required environment variables are set in the context of a local run. | ||
| */ | ||
| export function prepareEnvironment() { |
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.
Could the name of this be something like prepareLocalRunEnvironment so it's more clear what it does?
Also, this isn't called at the start of the uploadSarif action. I guess the reason for that is that you'd never want to run that action in local mode, however I'm worried it could lead to bugs or misunderstandings in the future if someone modifies this function and assumes it's there.
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.
Sure.
src/finalize-db.ts
Outdated
| await runQueries(databaseFolder, sarifFolder, config); | ||
|
|
||
| if ('true' === core.getInput('upload')) { | ||
| if ('true' === core.getInput('upload') && !util.isLocalRun()) { |
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.
Could you move this check to inside the upload_lib.upload method? That way it'll cover the uploadSarif action too.
Also from the point of view of testing more code in the action, it might be better to move this check to immediately before we make the action upload, in order to skip less code. The only major thing this would affect is the fingerprinting code, which might be nice to test but is not essential.
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.
Make sense.
| export const getApiClient = function() { | ||
| import { isLocalRun } from "./util"; | ||
|
|
||
| export const getApiClient = function(allowLocalRun = false) { |
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.
Thanks for adding this. I like that API calls are now safe by default in local mode.
b0ece22 to
105f781
Compare
|
Please could you update |
|
@sampart I added a section to the readme. Should I move that to contributing? |
|
Actually, moving it sounds like a great idea, thanks. Please don't fully replace what's there, as I think it's helpful that we also tell people about the option of pushing a branch (that's simpler than using act), but please do add the info in. I don't think we need it in the README if it's in the Contributing guide. Thanks! |
|
Right...this is a little different from the Running the action section. The instructions are for running from the main repository, but locally through act. So, to get this working, you need to run this from a repository different from this one. And it will used the current code in this repository. It's a subtle difference that I didn't call out in the README. I'll make that explicit. The main use case is to debug why a specific repository's codeql action is failing. |
|
Oh, I totally hadn't realised that distinction, thanks for mentioning. Would an end-user of the action use this to troubleshoot their own workflows that use the action? If so, README would be the right place for this, after all. |
|
@sampart Probably not useful for end users. So, I think contributing is the right place for it. When I discussed this earlier, the use case would be for maintainers or contributors to determine why a particular repository is failing by reproducing locally. |
|
@aeisenberg a bunch of js files have got into the |
|
Darn. Let me fix that. Not sure what I did. |
|
I'm out tomorrow and Monday. Please merge if you feel like this is good. Otherwise, I'll look at it when I get back. |
- Throw error if apiClient is retrieved for local runs and the `allowLocalRuns` flag isn't set. - Better debugging for `sendStatusReport`
This ensures that as much of the normal workflow occurs without doing the actual upload.
Co-authored-by: Joshua Hale <joshhale@github.com>
ffef8a4 to
36fef7c
Compare
This change allows the codeql-action to be run locally through
act.
In order to run the action locally, you need to do two things:
CODEQL_LOCAL_RUN: trueenvironment variable. The only wayI could figure out how to do this was to add it directly in the
workflow file in an
envblock. It should be possible to add itthrough a
.envfile and pass it toact, but I couldn't get itworking.
act -j codeql -s GITHUB_TOKEN=<MY_PAT>Setting the
CODEQL_LOCAL_RUNenv var will fill in missing env varsthat the action needs, but isn't set by
act. It will also avoidmaking api calls to github that would fail locally.
Merge / deployment checklist