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

Ensure queries[language] objects are initialized #518

Merged
merged 4 commits into from
May 21, 2021
Merged

Conversation

aibaars
Copy link
Collaborator

@aibaars aibaars commented May 21, 2021

This PR improves the CodeQL action in case a query suite is used that contains no queries. This is a bit of a corner case and resulted in an error like no property 'builtin' for 'undefined' instead of the more friendly message Unable to analyse ruby as no queries were selected for this language

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.

@@ -760,6 +760,14 @@ export async function getDefaultConfig(
logger
);
const queries: Queries = {};
for (const language of languages) {
if (queries[language] === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this always be true? The queries variable has just been defined to be {}.

@robertbrignull
Copy link
Contributor

So in this case, all the requisite codeql suites exist (i.e. ruby-code-scanning.qls, ruby-security-extended.qls, ruby-security-and-quality.qls) but they are empty? Otherwise I would have expected codeql to error trying to resolve the suite, but maybe it doesn't.

Another thing to consider to make the error message better is there's a chunk of code at the end of loadConfig that does

// The list of queries should not be empty for any language. If it is then
// it is a user configuration error.
for (const language of languages) {
  if (
    queries[language] === undefined ||
    (queries[language].builtin.length === 0 &&
      queries[language].custom.length === 0)
  ) {
    throw new Error(
      `Did not detect any queries to run for ${language}. ` +
        "Please make sure that the default queries are enabled, or you are specifying queries to run."
    );
  }
}

If you move that to initConfig so it runs when using the default config too then it'll give you a friendly error message quicker before it tries to build any code or run any queries.

@aibaars
Copy link
Collaborator Author

aibaars commented May 21, 2021

So in this case, all the requisite codeql suites exist (i.e. ruby-code-scanning.qls, ruby-security-extended.qls, ruby-security-and-quality.qls) but they are empty?

@robertbrignull Exactly. We don't have any queries yet that match the default query selectors.

@aibaars aibaars force-pushed the aibaars-no-queries branch from 5612390 to 17cf5fc Compare May 21, 2021 10:16
@aibaars
Copy link
Collaborator Author

aibaars commented May 21, 2021

@robertbrignull Thanks for the quick review. I addressed your comments.

Copy link
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
Should be fine

) {
throw new Error(
`Did not detect any queries to run for ${language}. ` +
"Please make sure that the default queries are enabled, or you are specifying queries to run."
Copy link
Contributor

Choose a reason for hiding this comment

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

You could change this error message to be more accurate for the case of ruby, but also I assume this situation of having empty suites will be temporary so it doesn't matter too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, indeed. The important thing is that the Action no long crashes with an undefined property error.

@aibaars aibaars force-pushed the aibaars-no-queries branch 2 times, most recently from 491900c to 74c4fa0 Compare May 21, 2021 10:38
@aibaars aibaars force-pushed the aibaars-no-queries branch from 74c4fa0 to 6a14acc Compare May 21, 2021 10:41
@aibaars aibaars enabled auto-merge May 21, 2021 10:47
@aibaars aibaars merged commit 1ad5a6c into main May 21, 2021
@aibaars aibaars deleted the aibaars-no-queries branch May 21, 2021 10:54
This was referenced May 21, 2021
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