GeezerVison

Bump artifacts dependencies #2461

Closed

3 tasks

angelapwen added the Update dependencies Trigger PR workflow to update dependencies label

github-actions Bot removed the Update dependencies Trigger PR workflow to update dependencies label

github-actions Bot commented Sep 13, 2024

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angelapwen marked this pull request as ready for review

September 13, 2024 19:22

angelapwen requested a review from a team as a code owner

September 13, 2024 19:22

angelapwen commented Sep 13, 2024

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aeisenberg reviewed

View reviewed changes

aeisenberg left a comment

Copy link

Copy Markdown

Contributor

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! 🎉 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angelapwen reacted with laugh emoji

All reactions

😄 1 reaction

Comment thread

src/analyze-action-post.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/debug-artifacts.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/start-proxy-action-post.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

angelapwen commented

View reviewed changes

Comment thread

src/upload-sarif-action-post.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

angelapwen commented Sep 16, 2024

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angelapwen added 2 commits

September 25, 2024 12:06


          Install actions/artifact@v1 as artifact-legacy and v2 as `artif…

76b799d

…act`


          Update original import statements to use artifact-legacy

6d887c1

angelapwen marked this pull request as draft

September 25, 2024 20:09

angelapwen added 2 commits

September 25, 2024 13:58


          Use new artifact dependency if not on GHES & feature flag enabled

4de3002


          Add PR check with feature flag enabled to test upgrade

35684ba

angelapwen force-pushed the angelapwen/bump-artifacts branch from 26b076b to 35684ba Compare

September 25, 2024 21:15


          Add changenote

4a5884a

angelapwen added the Update dependencies Trigger PR workflow to update dependencies label

Sep 25, 2024

github-actions Bot removed the Update dependencies Trigger PR workflow to update dependencies label

Sep 25, 2024

github-actions Bot commented Sep 25, 2024

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angelapwen marked this pull request as ready for review

September 25, 2024 21:33

angelapwen changed the title ~~Bump artifact dependencies~~ Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_UPGRADE enabled

Sep 25, 2024

henrymercer reviewed

View reviewed changes

henrymercer left a comment

Copy link

Copy Markdown

Contributor

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! ✨

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/debug-artifacts.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/feature-flags.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/debug-artifacts.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/start-proxy-action-post.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/start-proxy-action-post.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

.github/workflows/debug-artifacts-upgrade.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

angelapwen added 5 commits

September 26, 2024 13:19


          Refactor: remove argument array indirection and pass directly instead

8e4d3c2


          Rename feature flag to CODEQL_ACTION_ARTIFACT_V2_UPGRADE

bca51cd


          Add log messages at INFO level about which artifact version used

ce99e34


          Refactor: reuse getArtifactUploaderClient in start-proxy-action

0c5c594


          Update changenote

fbe39bd

angelapwen force-pushed the angelapwen/bump-artifacts branch from e96bc99 to fbe39bd Compare

September 26, 2024 20:49

angelapwen added the Update dependencies Trigger PR workflow to update dependencies label

github-actions Bot removed the Update dependencies Trigger PR workflow to update dependencies label


          Update checked-in dependencies

fd00289

github-actions Bot commented Sep 26, 2024

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot marked this pull request as draft

September 26, 2024 20:52

angelapwen marked this pull request as ready for review

September 26, 2024 20:52

angelapwen requested a review from henrymercer

September 26, 2024 22:56

angelapwen changed the title ~~Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_UPGRADE enabled~~ Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V2_UPGRADE enabled

aeisenberg reviewed

codeql-action/pr-checks/sync.py

View reviewed changes

aeisenberg left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

.github/workflows/debug-artifacts-upgrade.yml

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

aeisenberg Sep 26, 2024

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

angelapwen Sep 26, 2024

Copy link

Copy Markdown

Contributor Author

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:

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"
	]

. 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/debug-artifacts.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update CHANGELOG.md

f52494d

Co-authored-by: Andrew Eisenberg <[email protected]>

henrymercer reviewed

Sep 27, 2024

View reviewed changes

henrymercer left a comment

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/debug-artifacts.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

angelapwen added 2 commits

September 27, 2024 13:05


          Refer to download-artifact@v4 everywhere to reduce confusion

7c14037


          Merge remote-tracking branch 'origin' into angelapwen/upgrade-artifac…

…ts-rebase

angelapwen commented Sep 27, 2024

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

henrymercer previously approved these changes

Sep 30, 2024

View reviewed changes

henrymercer left a comment

Copy link

Copy Markdown

Contributor

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/feature-flags.ts Outdated

		*/
		export enum Feature {
		ArtifactV2Upgrade = "artifact_v2_upgrade",
		ArtifactV4Upgrade = "artifact_v2_upgrade",

henrymercer Sep 30, 2024

Copy link

Copy Markdown

Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

angelapwen Sep 30, 2024

Copy link

Copy Markdown

Contributor Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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.",

henrymercer Sep 30, 2024

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

angelapwen Sep 30, 2024

Copy link

Copy Markdown

Contributor Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

angelapwen added 2 commits

September 30, 2024 16:08


          Update feature flag name

6b58e15


          Remove logging that mentions feature flags for clarity

d2642b6

angelapwen dismissed henrymercer’s stale review via

d2642b6

September 30, 2024 23:10


          Update CHANGELOG.md

f29ffe2

Co-authored-by: Henry Mercer <[email protected]>

henrymercer previously approved these changes

View reviewed changes

henrymercer left a comment

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angelapwen reacted with heart emoji

All reactions

❤️ 1 reaction


          Merge branch 'main' into angelapwen/bump-artifacts

29620a0

angelapwen dismissed henrymercer’s stale review via

29620a0

October 1, 2024 15:47

angelapwen requested a review from henrymercer

October 1, 2024 16:02

henrymercer approved these changes

View reviewed changes

angelapwen changed the title ~~Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V2_UPGRADE enabled~~ Bump artifact dependencies if CODEQL_ACTION_ARTIFACT_V4_UPGRADE enabled

angelapwen merged commit a196a71 into main