-
Notifications
You must be signed in to change notification settings - Fork 335
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
Allow workflow to be passed via an environment variable for default setup #1619
Conversation
6bd6528
to
eaf7e8c
Compare
eaf7e8c
to
41b19a1
Compare
41b19a1
to
599f492
Compare
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.
Looks good and found the appropriate debug log in the internal run (2nd attempt) linked, but assume you want @Daverlo's 👀 too!
A couple of questions:
- will we add integration tests for default setup at a later point?
- do we want to add a changenote since this is user facing?
Yes! Tracking this internally.
Good point, let's add a changenote for this. |
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.
Not sure why the PR check is failing though. Re-ran with debug mode.
Edit: looks like we might have a test flake, but because the re-run succeeded I'm not sure what it was!
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.
LGTM!
*/ | ||
export async function getWorkflowPath(): Promise<string> { |
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.
What is this this returning when using default setup?
I don't think it really matters as I only see it being used for getting the analysis key, and for default setup we are always passing a category, so this value should be overwritten.
But I'm wondering if this should highlight that there is no workflow file somehow?
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.
For default setup, this will return dynamic/github-code-scanning/codeql
, but as you say we get the analysis key directly from CODEQL_ACTION_ANALYSIS_KEY
. I think this is a good thing to highlight though, as this is part of some work I'd like us to do around nailing down in the code what context the CodeQL Action expects from default setup, in terms of the github
job context, environment variables, etc.
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.
At the moment, I don't think we want to fail if the workflow file isn't found — we want to preserve the way we've been generating the analysis key to avoid breaking advanced setups where the workflow file doesn't exist, e.g. reusable workflows or workflows executing in other repositories.
The default setup workflow is not checked into the repository, but the Action relies on the workflow file to make features like reporting failed runs work. We'd like to move to a more robust method of making features like reporting failed runs work, but this is hard to do without making a breaking change to the workflow file, and we aren't currently in a good position to do that.
Therefore as a medium term solution, default setup will pass its workflow file to the Action via the
CODE_SCANNING_WORKFLOW_FILE
environment variable.GitHub staff: see an example run and the resulting status page.
Merge / deployment checklist