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

Allow the codeql-action to run packages #545

Merged
merged 15 commits into from
Jun 10, 2021
Merged

Conversation

aeisenberg
Copy link
Contributor

This commit adds a packs option to the codeql-config.yml file. Users
can specify a list of ql packs to include in the analysis.

For a single language analysis, the packs property looks like this:

packs:
  - pack-scope/pack-name1@1.2.3
  - pack-scope/pack-name2   # no explicit version means download the latest

For multi-language analysis, you must key the packs block by lanaguage:

packs:
  cpp:
    - pack-scope/pack-name1@1.2.3
    - pack-scope/pack-name2
  java:
    - pack-scope/pack-name3@1.2.3
    - pack-scope/pack-name4

This implementation adds a new analysis run (alongside custom and
builtin runs). The unit tests indicate that the correct commands are
being run, but I have not actually tried this with a real CLI.

Also, convert instanceof Array to Array.isArray since that is
sightly better in some situations. See:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#instanceof_vs_isarray

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.

This commit adds a `packs` option to the codeql-config.yml file. Users
can specify a list of ql packs to include in the analysis.

For a single language analysis, the packs property looks like this:

```yaml
packs:
  - pack-scope/pack-name1@1.2.3
  - pack-scope/pack-name2   # no explicit version means download the latest
```

For multi-language analysis, you must key the packs block by lanaguage:

```yaml
packs:
  cpp:
    - pack-scope/pack-name1@1.2.3
    - pack-scope/pack-name2
  java:
    - pack-scope/pack-name3@1.2.3
    - pack-scope/pack-name4
```

This implementation adds a new analysis run (alongside custom and 
builtin runs). The unit tests indicate that the correct commands are
being run, but I have not actually tried this with a real CLI.

Also, convert `instanceof Array` to `Array.isArray` since that is
sightly better in some situations. See:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#instanceof_vs_isarray
@aeisenberg aeisenberg marked this pull request as draft June 3, 2021 22:49
Copy link
Contributor

@edoardopirovano edoardopirovano left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a few minor comments/questions

src/config-utils.ts Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
src/codeql.ts Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/config-utils.test.ts Outdated Show resolved Hide resolved
src/config-utils.ts Show resolved Hide resolved
src/config-utils.ts Outdated Show resolved Hide resolved
@adityasharad
Copy link
Contributor

Is now a good point to create a feature branch?

@aeisenberg
Copy link
Contributor Author

Sure. I'll create one now.

@aeisenberg aeisenberg changed the base branch from main to packs/v1 June 4, 2021 17:19
Use strings instead. They are easier to serialize and deserialize.
@aeisenberg aeisenberg force-pushed the aeisenberg/pack-run branch 2 times, most recently from dfde222 to 5135e45 Compare June 4, 2021 21:07
src/codeql.ts Outdated Show resolved Hide resolved
@aeisenberg aeisenberg force-pushed the aeisenberg/pack-run branch 2 times, most recently from 9985119 to 0aac515 Compare June 6, 2021 06:05
languages: Language[],
configFile: string
) {
const packs = {} as Packs;
Copy link
Contributor

@RA80533 RA80533 Jun 6, 2021

Choose a reason for hiding this comment

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

Should Packs be updated such that {} would be valid? This sort of pattern appears in a few other places in the codebase. It might be that empty objects are intended to be valid for various configuration types (in which case it would make sense to update those types to allow for such shapes to be valid).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the type definition wrong. Originally, it was:

export type Packs = Record<Partial<Language>, PackWithVersion[]>;

But it should have been:

export type Packs = Partial<Record<Language, PackWithVersion[]>>;


// Exported for testing
export function parsePacks(
packsByLanguage: string[] | Record<string, string[]> | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for packsByLanguage to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter is coming directly from the parsedYAML[PACKS_PROPERTY] property, parsed from the file. This field is optional and hence possibly undefined.

Copy link
Contributor

@RA80533 RA80533 Jun 8, 2021

Choose a reason for hiding this comment

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

Oh, there's a conditional block right above that call site that it should be brought up inside.

const packs = parsePacks(parsedYAML[PACKS_PROPERTY], languages, configFile);

1. Better malformed data guard for PackDownloadOutput
2. Fix Packs type
3. Remove TODO in init-action
@aeisenberg aeisenberg force-pushed the aeisenberg/pack-run branch 4 times, most recently from 045cb83 to f612b8c Compare June 8, 2021 18:08
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.

4 participants