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

add first_party_analysis boolean to all status reports #2111

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

nickfyson
Copy link
Contributor

@nickfyson nickfyson commented Feb 1, 2024

In order to define more granular metrics and define more appropriate SLOs we add a new field to the status reports uploaded by the CodeQL Action.

This field first_party_analysis is based on whether the init action has been used, which is only used for first party analysis. When a SARIF file has been generated by other means and submitted using the upload action, this is considered to be a third party analysis and will be treated differently when calculating SLOs. To ensure misconfigured workflows are not treated as third party, only the upload-sarif action can submit status reports that are not first-party.

See back-linked internal issue for example status reports, showing these changes in action.

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.

@nickfyson nickfyson force-pushed the nickfyson/first-or-third-party branch from febf195 to a7dc229 Compare February 20, 2024 14:58
@nickfyson nickfyson marked this pull request as ready for review February 20, 2024 16:05
@nickfyson nickfyson requested a review from a team as a code owner February 20, 2024 16:05
@nickfyson nickfyson requested a review from angelapwen February 20, 2024 16:06
henrymercer
henrymercer previously approved these changes Feb 21, 2024
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -41,6 +41,9 @@ export enum EnvVar {
/** Whether the CodeQL Action has already warned the user about low disk space. */
HAS_WARNED_ABOUT_DISK_SPACE = "CODEQL_ACTION_HAS_WARNED_ABOUT_DISK_SPACE",

/** Whether the init action has been run. */
INIT_ACTION_HAS_RUN = "CODEQL_INIT_ACTION_HAS_RUN",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Doesn't read as well but it's nice to have everything the Action defines prefixed by CODEQL_ACTION_

Suggested change
INIT_ACTION_HAS_RUN = "CODEQL_INIT_ACTION_HAS_RUN",
INIT_ACTION_HAS_RUN = "CODEQL_ACTION_INIT_HAS_RUN",

Comment on lines 66 to 77
process.env["CODEQL_ACTION_ANALYSIS_KEY"] = "analysis-key";
process.env["GITHUB_REF"] = "refs/heads/main";
process.env["GITHUB_REPOSITORY"] = "octocat/HelloWorld";
process.env["GITHUB_RUN_ATTEMPT"] = "2";
process.env["GITHUB_RUN_ID"] = "100";
process.env["GITHUB_SHA"] = "a".repeat(40);
process.env["ImageVersion"] = "2023.05.19.1";
process.env["RUNNER_OS"] = "macOS";
process.env["RUNNER_TEMP"] = tmpDir;

const getRequiredInput = sinon.stub(actionsUtil, "getRequiredInput");
getRequiredInput.withArgs("matrix").resolves("input/matrix");
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: consider creating a function for all of this as it's shared across a few tests

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 great to me ✨ thanks for the care in thinking about misconfigured workflows!

@nickfyson nickfyson merged commit 982d934 into main Feb 21, 2024
297 checks passed
@nickfyson nickfyson deleted the nickfyson/first-or-third-party branch February 21, 2024 16:52
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.

3 participants