Uh oh!
There was an error while loading. Please reload this page.
Allow the codeql-action to be run locally - #117
Conversation
| } | ||
| core.debug('Action is running locally.'); | ||
| if (!process.env.RUNNER_TEMP) { |
There was a problem hiding this comment.
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.
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.
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.
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.
Thanks for catching that.
aibaars
commented
Jul 21, 2020
Setting variables in a file named |
aeisenberg
commented
Jul 21, 2020
Hmmm...The .env did not work for me. I wonder if I'm using an older version than you. Also, are you on Linux? |
aibaars
commented
Jul 21, 2020
Yes, on linux. |
robertbrignull
commented
Jul 21, 2020
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 |
robertbrignull
commented
Jul 21, 2020
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 |
aeisenberg
commented
Jul 21, 2020
Yes, that is what I was proposing indirectly above. I could do something like this: exportconstgetApiClient=function(allowLocalRun=false){if(isLocalRun()&&!allowLocalRun){thrownewError('Invalid API call in local run');}
...And then for calls we know to be safe, we pass the |
3966be4 to
cd5c2aeCompareaeisenberg
commented
Jul 21, 2020
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 |
aeisenberg
commented
Jul 21, 2020
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
ed210a5Compare5f292ee to
b0ece22Comparerobertbrignull
commented
Jul 23, 2020
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 |
aeisenberg
commented
Jul 23, 2020
Thanks for the clarification. |
aeisenberg
commented
Jul 29, 2020
@robertbrignull is this a worthwhile thing to add to the action? |
robertbrignull
left a comment
There was a problem hiding this comment.
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.
| /** | ||
| * Ensures all required environment variables are set in the context of a local run. | ||
| */ | ||
| export function prepareEnvironment() { |
There was a problem hiding this comment.
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.
| await runQueries(databaseFolder, sarifFolder, config); | ||
| if ('true' === core.getInput('upload')) { | ||
| if ('true' === core.getInput('upload') && !util.isLocalRun()) { |
There was a problem hiding this comment.
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.
| export const getApiClient = function() { | ||
| import { isLocalRun } from "./util"; | ||
| export const getApiClient = function(allowLocalRun = false) { |
There was a problem hiding this comment.
Thanks for adding this. I like that API calls are now safe by default in local mode.
b0ece22 to
105f781Comparesampart
commented
Jul 30, 2020
Please could you update |
aeisenberg
commented
Jul 30, 2020
@sampart I added a section to the readme. Should I move that to contributing? |
sampart
commented
Jul 30, 2020
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. |
sampart
commented
Jul 30, 2020
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. |
Uh oh!
There was an error while loading. Please reload this page.
aeisenberg
commented
Jul 30, 2020
@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. |
robertbrignull
commented
Jul 30, 2020
@aeisenberg a bunch of js files have got into the |
aeisenberg
commented
Jul 30, 2020
Darn. Let me fix that. Not sure what I did. |
aeisenberg
commented
Jul 30, 2020
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. |
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
36fef7cCompare
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