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

Refactor: prepare debug artifacts for artifact upgrades #2475

Merged
merged 8 commits into from
Sep 13, 2024

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Sep 11, 2024

As we prepare to upgrade to upload-artifacts@v4, download-artifacts@v4, or @actions/artifacts@v2, we noticed that there's a breaking change in that we can no longer upload to the same artifact name anymore: actions/upload-artifact#478 — once an artifact upload is completed, that artifact directory is immutable.

This PR prepares all our uses of the artifact dependencies for an easier switchover, but does not yet bump the dependencies for the switchover. The change should be primarily a refactor: the existing debug artifacts PR checks continue to pass.

  • Previously, we uploaded SARIF artifacts in the analyze-post step and database and log artifacts in the init-post step. As we migrate to the updated artifact dependencies, we want switch to uploading all artifacts in the init-post step. This should be safe as it always runs last.
    • In order to upload all artifacts in one go and maintain the artifacts at the root of the debug directory, we first move SARIF artifacts to the database directory. This should not affect any other consumers of the SARIF file as this occurs in the init-post step.
  • Previously, we also uploaded combined SARIF artifacts, under the same artifact name, in both the analyze-post step and the upload-sarif-post step. This change ensures that these artifacts are uploaded at most once — in analyze-post if it is a first-party run and upload-sarif-post if it is a third-party run.
  • The combined SARIF artifacts are now uploaded under the artifact name combined-sarif-artifacts rather than upload-debug-artifacts for clarity.

Merge / deployment checklist

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

@angelapwen angelapwen force-pushed the angelapwen/refactor-debug-artifacts-upload branch from 67813f8 to d896359 Compare September 11, 2024 21:12
Previously, we uploaded SARIF artifacts in the `analyze-post` step and database and log artifacts in the `init-post` step. As we migrate to the updated `artifact` dependencies, we want to switch to uploading all artifacts in one step.

In order to upload all artifacts in one go and maintain the artifacts at the root of the debug directory, we first move SARIF artifacts to the database directory. This should not affect any other consumers of the SARIF file as this occurs in the `init-post` step.
Previously, we uploaded combined SARIF artifacts in both the `analyze-post` and `upload-sarif-post` steps. This change ensures that these artifacts are uploaded at most once — in `analyze-post` if it is a first-party run and `upload-sarif-post` if it is a third-party run.

This is a defensive check because as we upgrade to the new `artifact` dependencies we will not be able to upload artifacts to the same artifact directory.
@angelapwen angelapwen force-pushed the angelapwen/refactor-debug-artifacts-upload branch from d896359 to 4ba2440 Compare September 11, 2024 22:13
@angelapwen angelapwen changed the title Refactor: upload all available debug artifacts in init-post Refactor: prepare debug artifacts for artifact upgrades Sep 11, 2024
@angelapwen angelapwen marked this pull request as ready for review September 11, 2024 22:30
@angelapwen angelapwen requested a review from a team as a code owner September 11, 2024 22:30
@angelapwen angelapwen mentioned this pull request Sep 11, 2024
3 tasks
src/analyze-action-post-helper.ts Outdated Show resolved Hide resolved
`${lang}.sarif`,
);
fs.renameSync(sarifFile, sarifInDbLocation);
filesToUpload = filesToUpload.concat(sarifInDbLocation);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Avoids reassigning a variable and the declaration above can be turned into a const.

Suggested change
filesToUpload = filesToUpload.concat(sarifInDbLocation);
filesToUpload.push(...sarifInDbLocation);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense — I had been meaning to make this change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any reason to use the ... operator when I know it's a single file like the sarifInDbLocation variable?

src/debug-artifacts.ts Outdated Show resolved Hide resolved
);
// Upload SARIF artifacts if we determine that this is a third-party analysis run.
// For first-party runs, this artifact will be uploaded in the `analyze-post` step.
if (process.env[EnvVar.INIT_ACTION_HAS_RUN] !== "true") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file ever imported by another typescript file? It looks like this file is only referenced from upload-sarif/action.yml. If so, I don't think we need the if statement around the call to upload the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not imported elsewhere, butuploadSarifActionPostHelper.uploadArtifacts() is and I'm trying to make sure it's called at most once — it's called once here and once in analyze-post. So in upload-sarif-post we only upload these artifacts if we're in third-party analysis; and in analyze-post we only upload if we're in first-party analysis.

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.

Thanks for separating out the PRs — this is easy to review!

src/analyze-action-post.ts Outdated Show resolved Hide resolved
src/debug-artifacts.ts Outdated Show resolved Hide resolved
@angelapwen angelapwen merged commit fe22310 into main Sep 13, 2024
312 checks passed
@angelapwen angelapwen deleted the angelapwen/refactor-debug-artifacts-upload branch September 13, 2024 16:47
@github-actions github-actions bot mentioned this pull request Sep 19, 2024
8 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.

3 participants