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

Run ML-powered query pack ~0.3.0 on v2.9.3+ of the CLI #1087

Merged
merged 5 commits into from Jun 15, 2022

Conversation

TomBolton
Copy link
Contributor

This PR starts running the following versions of the ML-powered query pack:

  • Version ~0.3.0 of the query pack on v2.9.3+ of the CLI
  • Version ~0.2.0 of the query pack on v2.8.4 <= CLI < v2.9.3
  • Version ~0.1.0 of the query pack on CLI < v2.8.4

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.

@TomBolton
Copy link
Contributor Author

@henrymercer is this how we would like to start running ~0.3.0?

Specifically, I've assumed in this PR that if the CLI is below 2.9.3, but above 2.8.4, we use ~0.2.0; otherwise we use ~0.1.0. Is this correct?

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 logic looks right to me, however to merge it we'll need to do two things:

  1. Compile the TypeScript files to JavaScript — there are instructions for that here: https://github.com/github/codeql-action/blob/main/CONTRIBUTING.md#development-and-testing. You should end up checking in a src/util.ts and corresponding lib/util.js and lib/util.js.map files. Let me know if you have any problems!

  2. Add some tests for this functionality in config-utils.test.ts to verify that the right pack gets loaded for 2.9.3. This commit shows how we did this for 0.2.0: e26813c

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.

This looks generally sensible. And you'll also need to do what Henry suggests.

src/util.ts Outdated Show resolved Hide resolved
@TomBolton
Copy link
Contributor Author

This PR is almost ready for another look, but one of the Basic Checks and Runner tests is failing with a duplicate test title and I'm not sure how to fix it, as the tests I added appear to be unique.

Screenshot 2022-06-13 at 17 28 41

@aeisenberg
Copy link
Contributor

There is an undefined in the test title. It probably means that the test macro isn't sufficient for the new tests you have written. The test macro is the thing that allows you to generate variants of a single test with some slight changes. In this case, we are using mlPoweredQueriesMacro on line 1712. The title property is what generates the title. You need to make sure that all generated titles are distinct.

You will need to change how the title is generated to ensure this is how it works. You can run the tests locally to ensure the titles are unique before pushing up a change.

@TomBolton TomBolton force-pushed the tombolton/update-ml-pack branch 2 times, most recently from 18563f4 to 602d5a3 Compare June 14, 2022 14:44
@TomBolton TomBolton marked this pull request as ready for review June 14, 2022 15:36
@TomBolton TomBolton requested a review from a team as a code owner June 14, 2022 15:36
@TomBolton
Copy link
Contributor Author

Thanks for the help @aeisenberg!

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 reasonable, assuming all the tests pass.

src/config-utils.test.ts Outdated Show resolved Hide resolved
@TomBolton
Copy link
Contributor Author

@henrymercer these two PR checks (here and here), involving a multi-language repo on mac, keep getting cancelled during the go extraction.

I restarted each check to see if it was a transient problem, but it happened again. Any ideas why this might be happening?

@henrymercer
Copy link
Contributor

I've raised the flaky tests internally. It seems that another rerun has sorted things with one of the jobs, so I've kicked off another run of the other job too.

@TomBolton
Copy link
Contributor Author

Thanks for looking into it @henrymercer and doing the reruns!

@TomBolton TomBolton merged commit df05122 into main Jun 15, 2022
@TomBolton TomBolton deleted the tombolton/update-ml-pack branch June 15, 2022 14:55
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.

None yet

3 participants