FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Run ML-powered query pack `~0.3.0` on v2.9.3+ of the CLI by TomBolton · Pull Request #1087 · github/codeql-action · GitHub

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

Merged
TomBolton merged 5 commits into
mainfrom
tombolton/update-ml-pack
Jun 15, 2022
Merged

TomBolton merged 5 commits into
mainfrom
tombolton/update-ml-pack

Conversation

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

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

henrymercer left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aeisenberg left a comment
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/util.ts Outdated

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

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

Copy link
Copy Markdown
Contributor Author

Thanks for the help @aeisenberg!

aeisenberg left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks reasonable, assuming all the tests pass.

Comment thread src/config-utils.test.ts Outdated
TomBolton force-pushed the tombolton/update-ml-pack branch from 04a23ec to a27dc4f Compare June 15, 2022 10:42
TomBolton requested a review from henrymercer June 15, 2022 10:42

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

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

Copy link
Copy Markdown
Contributor Author

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

TomBolton merged commit df05122 into main Jun 15, 2022
TomBolton deleted the tombolton/update-ml-pack branch June 15, 2022 14:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL