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

Do not artificially restrict the set of supported languages #779

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

esbena
Copy link
Contributor

@esbena esbena commented Oct 14, 2021

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

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

@esbena esbena added enhancement New feature or request javascript Pull requests that update Javascript code labels Oct 14, 2021
@aeisenberg
Copy link
Contributor

This is generally a good idea, IMO, especially as we start building out more languages.

@@ -299,7 +299,7 @@ test("load non-empty input", async (t) => {

// And the config we expect it to parse to
const expectedConfig: configUtils.Config = {
languages: [Language.javascript],
languages: [KnownLanguage.javascript],
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 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.

@@ -1172,7 +1174,7 @@ function shouldCombinePacks(packsInput?: string): boolean {
function combinePacks(packs1: Packs, packs2: Packs): Packs {
const packs = {};
for (const lang of Object.keys(packs1)) {
packs[lang] = packs1[lang].concat(packs2[lang] || []);
packs[lang] = packs1[lang]?.concat(packs2[lang] || []);
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 is what we want to do in order to ensure we never set packs[lang] to undefined.

Suggested change
packs[lang] = packs1[lang]?.concat(packs2[lang] || []);
packs[lang] = (packs1[lang] || []).concat(packs2[lang] || []);

(process.env["CODEQL_EXTRACTOR_GO_BUILD_TRACING"] === "on" &&
language === Language.go)
language === KnownLanguage.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

So any non KnownLanguage is assumed to be a scanned language? Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The default could go either way though.
For full flexibility, the action could take a parameter that signals the desired mode.

@aeisenberg
Copy link
Contributor

Will the extra extractors need to be packaged with the distribution, or are you expecting extractors to be resolvable from user supplied qlpacks?

@esbena
Copy link
Contributor Author

esbena commented Oct 25, 2021

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)

Copy link

@walid97t walid97t left a comment

Choose a reason for hiding this comment

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

go ahead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants