Refactor: prepare debug artifacts for artifact upgrades
#2475
Conversation
angelapwen
force-pushed
the
angelapwen/refactor-debug-artifacts-upload
branch
from
67813f8
to
d896359
Compare
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
force-pushed
the
angelapwen/refactor-debug-artifacts-upload
branch
from
d896359
to
4ba2440
Compare
angelapwen
changed the title
init-postartifact upgrades
src/debug-artifacts.ts
Outdated
| `${lang}.sarif`, | ||
| ); | ||
| fs.renameSync(sarifFile, sarifInDbLocation); | ||
| filesToUpload = filesToUpload.concat(sarifInDbLocation); |
There was a problem hiding this comment.
minor: Avoids reassigning a variable and the declaration above can be turned into a const.
| filesToUpload = filesToUpload.concat(sarifInDbLocation); | |
| filesToUpload.push(...sarifInDbLocation); |
There was a problem hiding this comment.
Yep, makes sense — I had been meaning to make this change!
There was a problem hiding this comment.
Is there any reason to use the ... operator when I know it's a single file like the sarifInDbLocation variable?
| ); | ||
| // 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") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
More accurately describes what these artifacts are, rather than the step they're uploaded in.
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.
analyze-poststep and database and log artifacts in theinit-poststep. As we migrate to the updatedartifactdependencies, we want switch to uploading all artifacts in theinit-poststep. This should be safe as it always runs last.init-poststep.analyze-poststep and theupload-sarif-poststep. This change ensures that these artifacts are uploaded at most once — inanalyze-postif it is a first-party run andupload-sarif-postif it is a third-party run.combined-sarif-artifactsrather thanupload-debug-artifactsfor clarity.Merge / deployment checklist