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

Allow workflow to be passed via an environment variable for default setup #1619

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

henrymercer
Copy link
Contributor

@henrymercer henrymercer commented Mar 28, 2023

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

  • 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.

src/workflow.ts Outdated Show resolved Hide resolved
@henrymercer henrymercer force-pushed the henrymercer/default-setup-workflow branch from 6bd6528 to eaf7e8c Compare April 11, 2023 17:25
src/workflow.ts Outdated Show resolved Hide resolved
@henrymercer henrymercer force-pushed the henrymercer/default-setup-workflow branch from eaf7e8c to 41b19a1 Compare April 12, 2023 12:51
@henrymercer henrymercer force-pushed the henrymercer/default-setup-workflow branch from 41b19a1 to 599f492 Compare April 12, 2023 13:14
@henrymercer henrymercer marked this pull request as ready for review April 12, 2023 13:21
@henrymercer henrymercer requested a review from a team as a code owner April 12, 2023 13:21
@henrymercer henrymercer requested a review from Daverlo April 12, 2023 13:21
@henrymercer henrymercer changed the title Update workflow path for default setup Allow workflow to be passed via an environment variable for default setup Apr 12, 2023
angelapwen
angelapwen previously approved these changes Apr 12, 2023
Copy link
Contributor

@angelapwen angelapwen left a 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?

@henrymercer
Copy link
Contributor Author

  • will we add integration tests for default setup at a later point?

Yes! Tracking this internally.

  • do we want to add a changenote since this is user facing?

Good point, let's add a changenote for this.

Copy link
Contributor

@angelapwen angelapwen left a 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!

Copy link
Contributor

@Daverlo Daverlo left a 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> {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@henrymercer henrymercer merged commit d944b34 into main Apr 13, 2023
@henrymercer henrymercer deleted the henrymercer/default-setup-workflow branch April 13, 2023 09:17
@github-actions github-actions bot mentioned this pull request Apr 13, 2023
6 tasks
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.

4 participants