Skip to content

Split out CLI version tests to separate workflow#2729

Merged
charisk merged 7 commits intomainfrom
charisk/split-out-cli-tests
Aug 22, 2023
Merged

Split out CLI version tests to separate workflow#2729
charisk merged 7 commits intomainfrom
charisk/split-out-cli-tests

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Aug 21, 2023

We currently run all CLI integration tests for every supported version of the CodeQL CLI, for windows and linux and for every commit of every PR. This is wasteful and it also adds minutes and chance of hitting flakiness.

We've also not generally seen tests succeed on one version and fail on another (outside of flakes), so pragmatically it doesn't make a huge amount of sense to run these tests so often. There aren't a lot of code changes that are related to different CLI versions.

This PR changes the workflows so that instead of running the whole matrix every time, we:

  • Run only against the latest released CLI version for PR workflows
  • We have a daily run of the CLI version tests

If we're finding that we're missing legit failures on the new workflow, we can add a process to notify the team when a failure happens. I'm thinking that at the moment it's worth doing this as it may be too noisy (due to flakiness).

You can see the new workflow here: https://github.com/github/vscode-codeql/actions/runs/5923448364

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested a review from a team as a code owner August 21, 2023 08:58
@robertbrignull
Copy link
Copy Markdown
Contributor

We'll need to remove all the branch protection rules before we can merge this. Which is also a step that'll need to be removed from the CodeQL CLI release process docs.

But fun fact, you can see from "13 successful and 13 expected checks" that this PR reduces the number of CI workflow runs by 50% 😌

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

LGTM

Do we want/need more review on this from the CodeQL team, or are you happy to just go ahead?

Comment thread .github/workflows/main.yml Outdated
matrix:
os: [ubuntu-latest, windows-latest]
version: ${{ fromJson(needs.set-matrix.outputs.cli-versions) }}
version: ['${{ needs.get-latest-cli-version.outputs.cli-version }}']
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A thought, that I didn't think of before approving: don't include this in the matrix here. This will make the name of the CI job just CLI Test (ubuntu-latest) instead of CLI Test (ubuntu-latest, v2.14.2) and it will mean the job name won't keep changing over time as the CLI gets updated. This means we won't have to update the branch protection rules every time the CLI version changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good shout!

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Aug 21, 2023

Apologies I should have mentioned the plan in the PR description (because even I forgot about it for a minute 🤦 !). I was thinking of the following:

  • Get PR reviewed from our team (✅) to make sure we're on the same page
  • Check CodeQL experiences folks are happy with this change and create PRs to update the various docs on this
  • Remove protection rules
  • Merge PR

Copy link
Copy Markdown
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.

It's probably fine as it is, but I just want to be sure. Feel free to merge if the answers to my questions are both no.

Comment thread .github/workflows/cli-test.yml Outdated
@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Aug 22, 2023

@robertbrignull let me know if you're still happy with this PR to go ahead

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

I'm still happy with this 👍🏼

@charisk charisk merged commit 3094405 into main Aug 22, 2023
@charisk charisk deleted the charisk/split-out-cli-tests branch August 22, 2023 10:28
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