-
Notifications
You must be signed in to change notification settings - Fork 323
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 if CODEQL_ACTION_ARTIFACT_V4_UPGRADE
enabled
#2482
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 |
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
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 <aeisenberg@github.com>
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~ |
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
@@ -40,7 +40,7 @@ export interface FeatureEnablement { | |||
* Legacy features should end with `_enabled`. | |||
*/ | |||
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
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 <henrymercer@github.com>
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-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 change would immediately cause customers using v3 of
actions/download-artifact
in 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_UPGRADE
environment variable to true in their workflows as they upgradeactions/download-artifact
tov4
.At the beginning of November, we will set the default value of the feature flag to
true
and the upgraded version of the artifact dependencies will be the default.This PR:
@actions/artifact@v1
asartifact-legacy
and imports@actions/artifact@v2
asartifact
CODEQL_ACTION_ARTIFACT_V4_UPGRADE
environment variable to trueartifact
client on GHES or when theCODEQL_ACTION_ARTIFACT_V4_UPGRADE
flag is false; and otherwise the upgraded versionCODEQL_ACTION_ARTIFACT_V4_UPGRADE
to true, bumpingupload-artifact
anddownload-artifact
to v4.Merge / deployment checklist