Bump artifact dependencies #2482
Conversation
We can do this now that we have upgraded to `@actions/artifact@v2`; note that on GHES we are still using `@actions/artifact@v1`, but our PR checks are not running on GHES.
angelapwen
added
the
Update dependencies
github-actions
bot
removed
the
Update dependencies
|
Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks. |
|
💭 People who have workflows using |
src/analyze-action-post.ts
Outdated
| if (config !== undefined) { | ||
| await debugArtifacts.uploadCombinedSarifArtifacts(config); | ||
| } |
There was a problem hiding this comment.
Should we warn when config === undefined?
There was a problem hiding this comment.
I'm not sure actually: elsewhere when we see that config is undefined, it indicates that the init step didn't succeed successfully. Since this is the analyze-post step, the init step should have succeeded. So it seems unlikely that anyone will ever end up in this state.
That made me think of something else though.. will post a separate comment.
src/debug-artifacts.ts
Outdated
| sanitizeArifactName(`${artifactName}${suffix}`), | ||
| toUpload.map((file) => path.normalize(file)), | ||
| path.normalize(rootDir), | ||
| { | ||
| continueOnError: true, | ||
| // ensure we don't keep the debug artifacts around for too long since they can be large. | ||
| retentionDays: 7, |
There was a problem hiding this comment.
I think the arguments here and below are the same. If so, I'd like to make that explicit by only specifying them once. And if they aren't the same, I'd like it to be clear where the differences are. You could probably do something like:
const artifactUploader = (ghVariant === GitHubVariant.GHES
? artifactLegacy.create()
: new artifact.DefaultArtifactClient())
artifactUploader.uploadArtifact(...args...);There was a problem hiding this comment.
They're almost the same, except the new client no longer accepts continueOnError as an option 😬 I might be able to use the ternary operator to append the continueOnError only for the legacy client.
There was a problem hiding this comment.
Actually, it looks like the default behavior of the legacy client is to set continueOnError to true. So we don't have to specify it and the args can be the same!
src/start-proxy-action-post.ts
Outdated
| .create() | ||
| .uploadArtifact( | ||
| if (config.gitHubVersion.type === GitHubVariant.GHES) { | ||
| await artifactLegacy |
src/upload-sarif-action-post.ts
Outdated
| const config = await getConfig(getTemporaryDirectory(), logger); | ||
| if (config !== undefined) { | ||
| await debugArtifacts.uploadCombinedSarifArtifacts(config); | ||
| } |
There was a problem hiding this comment.
Thinking out loud: Since config gets set in init, and this is only supposed to run in third-party analysis runs, config will always be undefined here. So I think we actually need to determine the GH variant separately here.
|
Holding on this as I investigate internal reports that #2475 has caused some regressions |
We need to bump
actions/upload-artifactandactions/download-artifactto v4 in our PR checks, as v3 will be deprecated in November: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/To do so, we also need to bump
@actions/artifactto v2, but it is not yet supported on GHES: https://www.npmjs.com/package/@actions/artifact#v2---whats-newThis PR:
@actions/artifact@v1asartifact-legacyand imports@actions/artifact@v2asartifactupload-artifactanddownload-artifactto v4Merge / deployment checklist