Skip to content
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

Merged
merged 18 commits into from
Oct 1, 2024

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Sep 13, 2024

We need to bump actions/upload-artifact and actions/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-new

This 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 the CODEQL_ACTION_ARTIFACT_V4_UPGRADE environment variable to true in their workflows as they upgrade actions/download-artifact to v4.

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:

  • imports @actions/artifact@v1 as artifact-legacy and imports @actions/artifact@v2 as artifact
  • adds a feature flag, defaulting to false, that can be enabled by setting the CODEQL_ACTION_ARTIFACT_V4_UPGRADE environment variable to true
  • uses the legacy version of the artifact client on GHES or when the CODEQL_ACTION_ARTIFACT_V4_UPGRADE flag is false; and otherwise the upgraded version
  • adds a PR check that sets CODEQL_ACTION_ARTIFACT_V4_UPGRADE to true, bumping upload-artifact and download-artifact to v4.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@angelapwen angelapwen mentioned this pull request Sep 13, 2024
3 tasks
@angelapwen angelapwen added the Update dependencies Trigger PR workflow to update dependencies label Sep 13, 2024
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Sep 13, 2024
Copy link
Contributor

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@angelapwen angelapwen marked this pull request as ready for review September 13, 2024 19:22
@angelapwen angelapwen requested a review from a team as a code owner September 13, 2024 19:22
@angelapwen
Copy link
Contributor Author

💭 People who have workflows using download-artifacts@v3 will no longer be able to access the debug artifacts that way (they will need to use @v4) — I'm not sure how many users do that as usually the debug artifacts are more of a manual inspection sort of thing. If we think enough people do that we could add a changenote.

Copy link
Contributor

@aeisenberg aeisenberg left a 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 Show resolved Hide resolved
src/debug-artifacts.ts Outdated Show resolved Hide resolved
src/start-proxy-action-post.ts Outdated Show resolved Hide resolved
@angelapwen
Copy link
Contributor Author

Holding on this as I investigate internal reports that #2475 has caused some regressions

@angelapwen angelapwen marked this pull request as draft September 25, 2024 20:09
@angelapwen angelapwen added the Update dependencies Trigger PR workflow to update dependencies label Sep 25, 2024
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Sep 25, 2024
Copy link
Contributor

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@angelapwen angelapwen marked this pull request as ready for review September 25, 2024 21:33
@angelapwen angelapwen changed the title Bump artifact dependencies Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_UPGRADE enabled Sep 25, 2024
Copy link
Contributor

@henrymercer henrymercer left a 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! ✨

src/debug-artifacts.ts Outdated Show resolved Hide resolved
src/feature-flags.ts Outdated Show resolved Hide resolved
src/debug-artifacts.ts Outdated Show resolved Hide resolved
src/start-proxy-action-post.ts Outdated Show resolved Hide resolved
src/start-proxy-action-post.ts Outdated Show resolved Hide resolved
.github/workflows/debug-artifacts-upgrade.yml Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@angelapwen angelapwen added the Update dependencies Trigger PR workflow to update dependencies label Sep 26, 2024
@github-actions github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Sep 26, 2024
Copy link
Contributor

Pushed a commit to update the checked-in dependencies. Please mark the PR as ready for review to trigger PR checks.

@github-actions github-actions bot marked this pull request as draft September 26, 2024 20:52
@angelapwen angelapwen marked this pull request as ready for review September 26, 2024 20:52
@angelapwen angelapwen changed the title Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_UPGRADE enabled Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V2_UPGRADE enabled Sep 26, 2024
Copy link
Contributor

@aeisenberg aeisenberg left a 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.

Comment on lines +27 to +34
- stable-v2.13.5
- stable-v2.14.6
- stable-v2.15.5
- stable-v2.16.6
- stable-v2.17.6
- default
- linked
- nightly-latest
Copy link
Contributor

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.

Copy link
Contributor Author

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:

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"
]
. When we update the list in that file, we also update the manually created PR checks.

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.

CHANGELOG.md Outdated Show resolved Hide resolved
src/debug-artifacts.ts Outdated Show resolved Hide resolved
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Copy link
Contributor

@henrymercer henrymercer left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/debug-artifacts.ts Outdated Show resolved Hide resolved
@angelapwen
Copy link
Contributor Author

There is some kind of unrelated CI incident related to Docker so will have to re-run these checks later~

henrymercer
henrymercer previously approved these changes Sep 30, 2024
Copy link
Contributor

@henrymercer henrymercer left a 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!

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -40,7 +40,7 @@ export interface FeatureEnablement {
* Legacy features should end with `_enabled`.
*/
export enum Feature {
ArtifactV2Upgrade = "artifact_v2_upgrade",
ArtifactV4Upgrade = "artifact_v2_upgrade",
Copy link
Contributor

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?

Copy link
Contributor Author

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!

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.",
Copy link
Contributor

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.

Copy link
Contributor Author

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>
henrymercer
henrymercer previously approved these changes Oct 1, 2024
Copy link
Contributor

@henrymercer henrymercer left a 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.

@angelapwen angelapwen changed the title Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V2_UPGRADE enabled Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V4_UPGRADE enabled Oct 1, 2024
@angelapwen angelapwen merged commit a196a71 into main Oct 1, 2024
277 checks passed
@angelapwen angelapwen deleted the angelapwen/bump-artifacts branch October 1, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants