Skip to content

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Apr 28, 2022

Also, use newer versions of the integration test packs that allow us to run with the latest CLI.

There are a few things of note here:

  1. Previously, we had relied on the action to parse the pack name+version string and create a query suite for each string. This is no longer necessary (and not even possible if you add a path). Instead, we do some minimal checks to ensure the string is syntactically correct (further checks are done in the CLI itself). And instead we pass query specification strings around.
  2. Because of the above, we no longer need to create a synthetic query string for each pack specification. We can just run each query specification directly.
  3. We are running each specification in a separate command invocation because of command line length limitations in windows. I'm not really sure if this is a problem, but there was a comment in the code to that effect and I decided that we should run commands separately to be safe
  4. This has an affect on how we check for ML query packs, mostly a simplification.
  5. The qlpacks used for testing have their sources stored in our private repository and in order to update them, we need to edit them there. Perhaps we should move them to this repository.

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.

@aeisenberg aeisenberg requested a review from a team as a code owner April 28, 2022 04:00
@aeisenberg aeisenberg marked this pull request as draft April 28, 2022 04:00
@aeisenberg aeisenberg removed the request for review from a team April 28, 2022 04:00
@aeisenberg aeisenberg force-pushed the aeisenberg/packs-with-paths branch 4 times, most recently from b8e27d7 to f86f601 Compare April 28, 2022 18:34
Also, this cleans up our pack-related integration tests.
We are now testing with the most recent CLIs.
@aeisenberg aeisenberg force-pushed the aeisenberg/packs-with-paths branch from f86f601 to 06b15c2 Compare April 29, 2022 00:14
@aeisenberg aeisenberg changed the title [WIP] Allow running packs with paths Allow running packs with paths Apr 29, 2022
@aeisenberg aeisenberg marked this pull request as ready for review April 29, 2022 00:33
@aeisenberg aeisenberg requested a review from a team April 29, 2022 00:34
# use tr to replace newlines with spaces and xargs to trim leading and trailing whitespace
RULES="$(cat javascript.sarif | jq -r '.runs[0].results[].ruleId' | sort | tr "\n" " " | xargs)"
RULES="$(cat javascript.sarif | jq -r '.runs[0].results[].ruleId' | sort | tr "\n" " " | tr "\r" " " | xargs)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this also works (first character of string1 maps to first character of string2, and the second to the second, and so on):

tr "\r\n" "  "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh...didn't know tr worked that way. Thanks.

@aeisenberg
Copy link
Contributor Author

I'm about to make a minor change to this PR and I will ask for another review. Thanks for looking.

@aeisenberg aeisenberg force-pushed the aeisenberg/packs-with-paths branch from 0120296 to a73e506 Compare April 30, 2022 00:33
@aeisenberg
Copy link
Contributor Author

Note that some required checks have been removed and that's why this is not mergeable. I am going to remove these as required checks and then recreate the list of required as described here: https://github.com/github/codeql-core/issues/1405#issuecomment-896190163

I'll make sure these instructions are easier to find.

@aeisenberg aeisenberg merged commit 0235de0 into main May 2, 2022
@aeisenberg aeisenberg deleted the aeisenberg/packs-with-paths branch May 2, 2022 15:24
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