-
Notifications
You must be signed in to change notification settings - Fork 428
Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V4_UPGRADE enabled
#2482
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
Conversation
|
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 |
aeisenberg
left a comment
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! 🎉 😄
|
Holding on this as I investigate internal reports that #2475 has caused some regressions |
26b076b to
35684ba
Compare
|
Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks. |
CODEQL_ACTION_ARTIFACT_UPGRADE enabled
henrymercer
left a comment
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.
Thanks for the extra work here to get this behind a feature flag and provide a smooth upgrade path! ✨
e96bc99 to
fbe39bd
Compare
|
Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks. |
CODEQL_ACTION_ARTIFACT_UPGRADE enabledCODEQL_ACTION_ARTIFACT_V2_UPGRADE enabled
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.
Looks good. The test makes sense, but I just want to make sure it is easy to maintain.
| - stable-v2.13.5 | ||
| - stable-v2.14.6 | ||
| - stable-v2.15.5 | ||
| - stable-v2.16.6 | ||
| - stable-v2.17.6 | ||
| - default | ||
| - linked | ||
| - nightly-latest |
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.
Are we not testing 2.18 or 2.19? How will we maintain this list going forward? Same question for the set of versions below.
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.
We try to keep the list of versions in the checks that don't use the PR check generator consistent with the sync.py file:
codeql-action/pr-checks/sync.py
Lines 9 to 28 in 46e0c78
| defaultTestVersions = [ | |
| # The oldest supported CodeQL version. If bumping, update `CODEQL_MINIMUM_VERSION` in `codeql.ts` | |
| "stable-v2.13.5", | |
| # The last CodeQL release in the 2.14 series. | |
| "stable-v2.14.6", | |
| # The last CodeQL release in the 2.15 series. | |
| "stable-v2.15.5", | |
| # The last CodeQL release in the 2.16 series. | |
| "stable-v2.16.6", | |
| # The last CodeQL release in the 2.17 series. | |
| "stable-v2.17.6", | |
| # The default version of CodeQL for Dotcom, as determined by feature flags. | |
| "default", | |
| # The version of CodeQL shipped with the Action in `defaults.json`. During the release process | |
| # for a new CodeQL release, there will be a period of time during which this will be newer than | |
| # the default version on Dotcom. | |
| "linked", | |
| # A nightly build directly from the our private repo, built in the last 24 hours. | |
| "nightly-latest" | |
| ] |
default is currently 2.19.0, so looks like we'll need to add 2.18.6 to the list. I'll open up a separate internal issue to track that.
Co-authored-by: Andrew Eisenberg <[email protected]>
henrymercer
left a comment
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.
This looks great. Just a couple of comments about whether we can avoid the confusion between the npm and Action versions by always talking about the Action versions, in line with the public changelog posts.
|
There is some kind of unrelated CI incident related to Docker so will have to re-run these checks later~ |
henrymercer
left a comment
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! A couple of small suggestions, but this looks good!
src/feature-flags.ts
Outdated
| */ | ||
| export enum Feature { | ||
| ArtifactV2Upgrade = "artifact_v2_upgrade", | ||
| ArtifactV4Upgrade = "artifact_v2_upgrade", |
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 also update the name of the feature flag?
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.
Good point — forgot to do that because we don't really use that value. Fixed!
src/debug-artifacts.ts
Outdated
| } else if (!(await features.getValue(Feature.ArtifactV4Upgrade))) { | ||
| logger.info( | ||
| "Uploading debug artifacts using the `@actions/artifact@v1` client because the value of the relevant feature flag is false. To use the `v2` version of the client, set the `CODEQL_ACTION_ARTIFACT_V2_UPGRADE` environment variable to true.", | ||
| "Debug artifacts can be consumed with `actions/download-artifact@v3` because the value of the relevant feature flag is false. To use the `actions/download-artifact@v4`, set the `CODEQL_ACTION_ARTIFACT_V4_UPGRADE` environment variable to true.", |
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'd suggest we drop "because the value of the relevant feature flag is false" here, since customers shouldn't need to know about feature flags.
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.
Makes sense 👍 to them it's just behind an environment variable. Done!
Co-authored-by: Henry Mercer <[email protected]>
henrymercer
left a comment
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.
Looks good, just one merge conflict to resolve in the changelog.
CODEQL_ACTION_ARTIFACT_V2_UPGRADE enabledCODEQL_ACTION_ARTIFACT_V4_UPGRADE enabled
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 change would immediately cause customers using v3 of
actions/download-artifactin their workflows to see failures, so we are gating it behind an opt-in feature flag for another month. Customers will be able to set theCODEQL_ACTION_ARTIFACT_V4_UPGRADEenvironment variable to true in their workflows as they upgradeactions/download-artifacttov4.At the beginning of November, we will set the default value of the feature flag to
trueand the upgraded version of the artifact dependencies will be the default.This PR:
@actions/artifact@v1asartifact-legacyand imports@actions/artifact@v2asartifactCODEQL_ACTION_ARTIFACT_V4_UPGRADEenvironment variable to trueartifactclient on GHES or when theCODEQL_ACTION_ARTIFACT_V4_UPGRADEflag is false; and otherwise the upgraded versionCODEQL_ACTION_ARTIFACT_V4_UPGRADEto true, bumpingupload-artifactanddownload-artifactto v4.Merge / deployment checklist