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

Enable a controlled switchover between CodeQL releases #1475

Merged
merged 29 commits into from
Jan 19, 2023

Conversation

henrymercer
Copy link
Contributor

@henrymercer henrymercer commented Jan 10, 2023

This PR adds functionality that we will use to enable a controlled switchover between CodeQL releases. Specifically, we would like to ensure that each repository running CodeQL transitions from the previous CodeQL release to the new CodeQL release once. We want to avoid repositories flip-flopping between CodeQL releases during the rollout of a new CodeQL release, specifically during the Actions runner image update process, as this creates alert churn.

To do this, we decide the default version of CodeQL on Dotcom using feature flags rather than by using what's in the toolcache. Specifically, we look at all the default_codeql_version_x_enabled feature flags and pick the latest enabled version.

One niggle with this approach is that if the CLI version we need isn't in the toolcache, there isn't yet a nice way to find the CodeQL Bundle release corresponding to that CLI version. In the medium term, we'll consider tagging each CodeQL bundle release with a CLI version number, for instance codeql-bundle-v2.12.0. In the meantime, we add a special asset to the release cli-version-2.12.0.txt specifying the version number. The contents of this asset don't matter, since we don't want to have to download anything.

GHES behaviour is unchanged: we account for the new toolcache format, but we continue to use the CodeQL bundle release specified within the Action (in defaults.json) by default. We also continue to allow CodeQL bundles that have been baked into Actions runner images to override the version in defaults.json.

Commit-by-commit-review recommended.

TODO before merging

  • Add a changelog note for the impact on the layout of CodeQL within the Actions runner images.
  • Adapt the functionality that enables using the toolcache for manually specified bundle URLs to the new runner image structure. This will require a search through toolcache versions or a bundle tag name to CodeQL CLI version lookup.
  • Add some more unit tests for setting up CodeQL. Also require an owner and a repo in the GitHub Releases API mocking as a regression test.

Merge / deployment checklist

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

@henrymercer henrymercer force-pushed the henrymercer/controlled-switchover branch from fa0e8d6 to a9eb86f Compare January 11, 2023 15:59
Base automatically changed from henrymercer/fix-ghae-setup-test to main January 11, 2023 17:14
@henrymercer henrymercer marked this pull request as ready for review January 11, 2023 19:10
@henrymercer henrymercer requested a review from a team as a code owner January 11, 2023 19:11
@henrymercer henrymercer force-pushed the henrymercer/controlled-switchover branch from a9eb86f to e8c12e1 Compare January 11, 2023 19:11
@henrymercer
Copy link
Contributor Author

I've added some things we need to do before merging this to the PR description, but I think this is ready for an initial look.

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through commit-by-commit and everything makes sense to me. Just a few comments for now, will take another 👀 tomorrow!

src/util.ts Outdated Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/feature-flags.test.ts Show resolved Hide resolved
src/setup-codeql.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@aeisenberg
Copy link
Contributor

aeisenberg commented Jan 12, 2023

One niggle with this approach is that if the CLI version we need isn't in the toolcache, there isn't yet a nice way to find the CodeQL Bundle release corresponding to that CLI version. In the medium term, we'll consider tagging each CodeQL bundle release with a CLI version number, for instance codeql-bundle-v2.12.0. In the meantime, we add a special asset to the release cli-version-2.12.0.txt specifying the version number. The contents of this asset don't matter, since we don't want to have to download anything.

Could we add a json file in this repo that has mappings from bundle release number to version number or URL? This would be helpful in general for our users since there's no easy way for them to figure out this mapping right now without downloading and running codeql version.

This could go in defaults.json, which we're already updating for each release.

@angelapwen
Copy link
Contributor

👍 on new additions. CI failure is due to known Swift autobuild hang and I've rerun.

@henrymercer
Copy link
Contributor Author

Could we add a json file in this repo that has mappings from bundle release number to version number or URL? This would be helpful in general for our users since there's no easy way for them to figure out this mapping right now without downloading and running codeql version.

This could go in defaults.json, which we're already updating for each release.

I have a slight preference for keeping the GitHub Releases the source of truth:

  • This aligns better with our medium term goal of changing the version numbering system of bundle releases to include the CLI version number. Once we've done that a mapping in defaults.json would be redundant.
  • We can automatically add the CLI version marker file to the bundle release as part of the publishing process, saving a bit of manual work.

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.

Given the complexity of this change and the fact that the actions changelog is not very visible to users, are you considering adding this to a changelog post?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/setup-codeql.ts Outdated Show resolved Hide resolved
@henrymercer
Copy link
Contributor Author

Given the complexity of this change and the fact that the actions changelog is not very visible to users, are you considering adding this to a changelog post?

I think a changelog post would be a good idea. Will discuss with Alona once we've agreed on a good Action changelog note.

angelapwen
angelapwen previously approved these changes Jan 17, 2023
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ from me on the code changes, but I believe there is still ongoing discussion about the inclusion of the section for advanced users in the changelog note.

angelapwen
angelapwen previously approved these changes Jan 18, 2023
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but looks like merge conflicts need to be resolved 😄

@henrymercer
Copy link
Contributor Author

Hmm since we already have merge conflicts here, it might make sense to get #1492 in first and then rebase on that, as I expect the TypeScript update will probably create some more conflicts.

@henrymercer
Copy link
Contributor Author

TypeScript PR is failing on Windows, so let's do this PR first instead.

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the PR checks are failing with parsing SARIF on Windows but it should be unrelated to this PR 🤔

@henrymercer henrymercer enabled auto-merge January 19, 2023 09:41
@henrymercer henrymercer merged commit 60e5868 into main Jan 19, 2023
@henrymercer henrymercer deleted the henrymercer/controlled-switchover branch January 19, 2023 09:42
@github-actions github-actions bot mentioned this pull request Jan 26, 2023
6 tasks
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.

4 participants