-
Notifications
You must be signed in to change notification settings - Fork 315
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
Bump artifact dependencies #2482
base: main
Are you sure you want to change the base?
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Some suggestions inline.
+2,174,754 −72,459
Productive day! 🎉 😄
src/analyze-action-post.ts
Outdated
if (config !== undefined) { | ||
await debugArtifacts.uploadCombinedSarifArtifacts(config); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we warn when config === undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as above.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-artifact
andactions/download-artifact
to 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/artifact
to v2, but it is not yet supported on GHES: https://www.npmjs.com/package/@actions/artifact#v2---whats-newThis PR:
@actions/artifact@v1
asartifact-legacy
and imports@actions/artifact@v2
asartifact
upload-artifact
anddownload-artifact
to v4Merge / deployment checklist