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

Fix the qlpack json schema #3527

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Mar 28, 2024

This schema was incorrectly tagging defaultSuite properties that aren't arrays as errors. The error that needs to be fixed is that the CLI leniently reads yaml. Any property that is typed as an array in the schema is also acceptable as a single element.

For example, the defaultSuite can either be an array of instructions or a single instruction.

There are also missing fields in the qlpack type, but I don't think that matters for schema validation.

Note that this fixes a bug in the test explorer where qlpacks with defaultSuite properties that are not arrays would cause the test explorer to avoid showing any test cases in that qlpack.

Checklist

  • 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.

@aeisenberg aeisenberg requested a review from a team as a code owner March 28, 2024 23:29
@aeisenberg aeisenberg force-pushed the aeisenberg/fix-qlpack-schema branch from d8dd34a to bb23de9 Compare March 29, 2024 04:33
@aeisenberg aeisenberg requested a review from a team as a code owner March 29, 2024 04:33
Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I think we should be more lenient on the include/exclude properties, but the other changes look good to me.

extensions/ql-vscode/src/packaging/suite-instruction.ts Outdated Show resolved Hide resolved
This schema was incorrectly tagging `defaultSuite` properties that
aren't arrays as errors.

I think there are more, similar errors in this schema, but I only fixed
this one so that I can get `github/semmle-code` working with test
discovery.
@aeisenberg aeisenberg force-pushed the aeisenberg/fix-qlpack-schema branch from d643f82 to 44c3c29 Compare April 3, 2024 03:22
@aeisenberg aeisenberg requested a review from koesie10 April 3, 2024 03:22
@aeisenberg aeisenberg merged commit 00c0e70 into github:main Apr 3, 2024
15 checks passed
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.

2 participants