Skip to content

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jul 20, 2020

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:

  1. Add the CODEQL_LOCAL_RUN: true environment variable. The only way
    I could figure out how to do this was to add it directly in the
    workflow file in an env block. It should be possible to add it
    through a .env file and pass it to act, but I couldn't get it
    working.
  2. Run this command act -j codeql -s GITHUB_TOKEN=<MY_PAT>

Setting the CODEQL_LOCAL_RUN env var will fill in missing env vars
that the action needs, but isn't set by act. It will also avoid
making api calls to github that would fail locally.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

src/util.ts Outdated
}

core.debug('Action is running locally.');
if (!process.env.RUNNER_TEMP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that.

@aibaars
Copy link
Contributor

aibaars commented Jul 21, 2020

Setting variables in a file named .env seems to work for me. Note that the flag is just a filename, which is resolved against the current working directory.

@aeisenberg
Copy link
Contributor Author

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
Copy link
Contributor

aibaars 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?

Yes, on linux.

@robertbrignull
Copy link
Contributor

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 getApiClient method.

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Jul 21, 2020

@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 apiClient that we need https://github.com/github/codeql-action/blob/main/src/config-utils.ts#L404. Though, I could probably still explicitly fail other requests if they are invoked from local mode.

@robertbrignull
Copy link
Contributor

I believe that is already handled here: https://github.com/github/codeql-action/pull/117/files#diff-8cd4968b81985f0efc2053eabc37db6dR263

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.

There is at least one call to apiClient that we need https://github.com/github/codeql-action/blob/main/src/config-utils.ts#L404. Though, I could probably still explicitly fail other requests if they are invoked from local mode.

I haven't used act before. Presumably it still runs in the context of some repository? In which case I expect that call should work as normal. I do wonder if it would make for a safer situation if we took the stance of denying all HTTP requests in local mode, and allowing only the ones we know to be safe, instead of the current situation which is the opposite.

@aeisenberg
Copy link
Contributor Author

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 true flag. And for calls that aren't, we will fail-fast and then need to handle them differently.

@aeisenberg aeisenberg force-pushed the aeisenberg/local-runs branch from 3966be4 to cd5c2ae Compare July 21, 2020 19:04
@aeisenberg
Copy link
Contributor Author

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 codeql action is not being run for local pull requests?

@aeisenberg
Copy link
Contributor Author

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

@aeisenberg aeisenberg force-pushed the aeisenberg/local-runs branch from ac386f7 to ed210a5 Compare July 22, 2020 20:59
@aeisenberg aeisenberg changed the base branch from v1 to main July 22, 2020 21:05
@aeisenberg aeisenberg force-pushed the aeisenberg/local-runs branch from 5f292ee to b0ece22 Compare July 22, 2020 21:49
@robertbrignull
Copy link
Contributor

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 main branch so this reintroduced some alerts to that branch that had been fixed.

@aeisenberg
Copy link
Contributor Author

Thanks for the clarification.

@aeisenberg
Copy link
Contributor Author

@robertbrignull is this a worthwhile thing to add to the action?

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.

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() {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

await runQueries(databaseFolder, sarifFolder, config);

if ('true' === core.getInput('upload')) {
if ('true' === core.getInput('upload') && !util.isLocalRun()) {
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

@sampart
Copy link
Contributor

sampart commented Jul 30, 2020

Please could you update CONTRIBUTING.md as part of this - there's a Running the action section there. Thanks!

@aeisenberg
Copy link
Contributor Author

@sampart I added a section to the readme. Should I move that to contributing?

@sampart
Copy link
Contributor

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!

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Jul 30, 2020

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
Copy link
Contributor

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.

@aeisenberg
Copy link
Contributor Author

@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
Copy link
Contributor

@aeisenberg a bunch of js files have got into the src directory somehow. They should be in lib, so I'm not sure how that happened.

@aeisenberg
Copy link
Contributor Author

Darn. Let me fix that. Not sure what I did.

@aeisenberg
Copy link
Contributor Author

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.

aeisenberg and others added 7 commits August 4, 2020 11:02
- 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>
@aeisenberg aeisenberg force-pushed the aeisenberg/local-runs branch from ffef8a4 to 36fef7c Compare August 4, 2020 18:02
@aeisenberg aeisenberg merged commit 42235cc into github:main Aug 4, 2020
@github-actions github-actions bot mentioned this pull request Aug 10, 2020
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