Do not artificially restrict the set of supported languages - #779
Conversation
Sorry, something went wrong.
|
This is generally a good idea, IMO, especially as we start building out more languages. |
Sorry, something went wrong.
| // And the config we expect it to parse to | ||
| const expectedConfig: configUtils.Config = { | ||
| languages: [Language.javascript], | ||
| languages: [KnownLanguage.javascript], |
There was a problem hiding this comment.
I think we should have a new unit test here for a language is not a known language, but it is valid since it was returned by a mocked call to codeql resolve languages.
Sorry, something went wrong.
| const packs = {}; | ||
| for (const lang of Object.keys(packs1)) { | ||
| packs[lang] = packs1[lang].concat(packs2[lang] || []); | ||
| packs[lang] = packs1[lang]?.concat(packs2[lang] || []); |
There was a problem hiding this comment.
I think this is what we want to do in order to ensure we never set packs[lang] to undefined.
| packs[lang] = packs1[lang]?.concat(packs2[lang] || []); | |
| packs[lang] = (packs1[lang] || []).concat(packs2[lang] || []); |
Sorry, something went wrong.
| [KnownLanguage.cpp, KnownLanguage.java, KnownLanguage.csharp].includes(language as any) || | ||
| (process.env["CODEQL_EXTRACTOR_GO_BUILD_TRACING"] === "on" && | ||
| language === Language.go) | ||
| language === KnownLanguage.go) |
There was a problem hiding this comment.
So any non KnownLanguage is assumed to be a scanned language? Is that correct?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes. The default could go either way though.
For full flexibility, the action could take a parameter that signals the desired mode.
Sorry, something went wrong.
|
Will the extra extractors need to be packaged with the distribution, or are you expecting extractors to be resolvable from user supplied qlpacks? |
Sorry, something went wrong.
|
If you modify the codeql search path appropriately, you can use your own extractor in the action. We do not document how though. (See https://github.com/github/codeql-ql/blob/main/.github/workflows/nightly-changes.yml#L82) |
Sorry, something went wrong.
walid97t
left a comment
There was a problem hiding this comment.
go ahead
Sorry, something went wrong.
Description
The hardcoded set of supported languages in languages.ts makes it hard to experiment with new languages as custom branch with a modified languages.ts is required for every new language.
This PR makes it easier to experiment with new languages by making the codeql-action indifferent to the language choice, except for some additional support for known languages.
Implementation
The old implementation throws an early error if the user-provided language is not in the hardcoded set. As an alternative, a Language is now just a lowercase string, and an early error is only produced if that language is not in the output of codeql resolve languages. The old Language enum is now named KnownLanguage, and is used in the places where additional support for a specific language is desired.
This should be fully backwards compatible, as the implementation now should throw an early exception in fewer situations. This may be detrimental to how early and well users are informed of input mistakes. On the other hand, it is consistent with how the provided codeql and environment behave.
PMs may want to keep the tight coupling between the codeql-action and the GitHub-controlled CodeQL repositories, I do not.
Testing
I will use this branch for some internal experiments for a short while. Hopefully, the many changes wont result in too many conflicts when we are ready to merge.
Merge / deployment checklist