Move config parsing earlier + add to codeql search path #109

robertbrignull · 2020-07-13T13:59:13Z

This change started out just trying to add --search-path to the CodeQL args when resolving queries, however in doing that I wasn't able to get the data where I needed it nicely. A simpler but cruder answer could have been to stuff some extra fields in the objects already being passed around, but that didn't seem ideal to me. Instead this PR restructures how we parse and process the config considerably to move everything sooner and to one place.

The main changes are:

All config parsing and subsequent processing happens in the init action. Previously it was mostly in the init action, but there was a large chunk in the analyze action where we checked out external repositories and converted the user-supplied list of queries and query suites to run into an explicit list of query files.
The data stored in the internal config file that persists between actions has changed. Some extra data has been added like the list of languages. Some data has changed format, such as the set of queries which is now an explicit array of filepaths to existing ql files.

This is quite a big change, but I think is has a few benefits:

The user gets notified of problems sooner, because we validate everything in the init action. Previously if you specified queries from a remote repository but the repository or file path didn't exist, then you wouldn't find out until after the code has been built.
All config parsing is done in one place. I hope this makes it easier to us to understand and develop. It also makes it easier to do features like adding the CodeQL --search-path because all of the data is available.
By storing more data and more explicit data in the internal config file, and treating the data as immutable, it makes the other actions simpler.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.

sampart

This is a really great improvement. All my comments are stylistic, so I'm approving this. Thank you!

sampart · 2020-07-14T14:57:33Z

src/codeql.ts

 }

+/**
+ * Stores the CodeQL object, and is populated by `setupCodeQL`.


It might also be set by getCodeQL; I don't know if you need to mention that.

sampart · 2020-07-14T15:03:48Z

src/codeql.ts

+  return partialCodeql[methodName];
+}
+
+export function setCodeQL(partialCodeql: Partial<CodeQL>) {


Do you need a comment saying "Only for use in tests?"

sampart · 2020-07-14T15:09:46Z

src/config-utils.ts

+/**
+ * Get the file path where the parsed config will be stored.
+ */
+export function getParsedConfigFile(): string {


getParsedConfigFile sounds to me like you're returning the configuration itself. What about getPathToParsedConfigFile?

If you do this, I guess the previous method should be renamed to something like getPathToParsedConfigFileFolder, although that does sound a bit clunky.

sampart · 2020-07-14T15:10:40Z

src/config-utils.ts

+  return languages;
+}
+
+export async function getBlankConfig(): Promise<Config> {


Would this be better as getDefaultConfig, since it does have content? That would also bring parity with addDefaultQueries.

sampart · 2020-07-14T15:13:18Z

src/config-utils.test.ts


-    // And the contents should parse correctly to the config that was returned
-    t.deepEqual(fs.readFileSync(configFile, 'utf8'), JSON.stringify(config));
+    // And the same config should be returned again


"again" makes it sound like you're calling this function for the second time, but you're not

Suggested change

// And the same config should be returned again

// And that same newly-initialised config should now be returned by getConfig

sampart · 2020-07-15T09:18:56Z

src/config-utils.ts

+  queryUses: string) {
+
+  // The logic for parsing the string is based on what actions does for
+  // parsing the 'uses' actions in the workflow file


Does this comment relate just to the 4 lines that follow? If not, I'd put it in the function-level comment.

sampart · 2020-07-15T09:29:53Z

src/config-utils.ts


+// Property names from the user-supplied config file.
 const NAME_PROPERTY = 'name';
 const DISPLAY_DEFAULT_QUERIES_PROPERTY = 'disable-default-queries';


I know this wasn't added in this PR, but you're making use of it in your changed code, and that code would be much easier to read if it was called DISABLE_DEFAULT_QUERIES_PROPERTY.

Consider this bit, for example:

if (!parsedYAML[DISPLAY_DEFAULT_QUERIES_PROPERTY]) { await addDefaultQueries(languages, queries); }

Thanks for spotting that. I think that must just be a slip of the keyboard from when that property was originally added.

sampart · 2020-07-15T09:32:19Z

src/config-utils.ts

+ * a default config. The parsed config is then stored to a known location.
+ */
+export async function initConfig(): Promise<Config> {
+  let configFile = core.getInput('config-file');


Suggested change

let configFile = core.getInput('config-file');

const configFile = core.getInput('config-file');

sampart · 2020-07-15T09:37:30Z

src/external-queries.test.ts

-    await externalQueries.checkoutExternalQueries(config);
+    await externalQueries.checkoutExternalRepository("github/codeql-go", "df4c6869212341b601005567381944ed90906b6b");

    // COPYRIGHT file existed in df4c6869212341b601005567381944ed90906b6b but not in master


Suggested change

// COPYRIGHT file existed in df4c6869212341b601005567381944ed90906b6b but not in master

// COPYRIGHT file existed in df4c6869212341b601005567381944ed90906b6b but not in main

The default branch on https://github.com/github/codeql-go is still called master. I'll make the comment more clear that it's talking about the default branch without naming it.

sampart · 2020-07-15T09:38:40Z

src/external-queries.ts


-export async function checkoutExternalQueries(config: configUtils.Config) {
+/**
+ * Checkout a repository at the given ref, and return the directory of the checkout.


Suggested change

* Checkout a repository at the given ref, and return the directory of the checkout.

* Check out repository at the given ref, and return the directory of the checkout.

Removing the "a" makes it clearer how you're using the repository parameter.

move config parsing earlier + add to codeql search path

da3d6d2

joshhale requested a review from sampart July 13, 2020 14:51

robertbrignull assigned sampart Jul 14, 2020

sampart approved these changes Jul 15, 2020

View reviewed changes

robertbrignull added 3 commits July 15, 2020 17:14

Merge branch 'main' into codeql_search_path

7bb6ac6

address review comments

b86c370

await promise

d138b00

robertbrignull merged commit b673c57 into main Jul 16, 2020

robertbrignull deleted the codeql_search_path branch July 16, 2020 08:17

github-actions bot mentioned this pull request Jul 20, 2020

Merge main into v1 #114

Merged

robertbrignull mentioned this pull request Jul 21, 2020

Default queries fix + reset env vars in tests #120

Merged

2 tasks

github-actions bot mentioned this pull request Jul 27, 2020

Merge main into v1 #126

Merged

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/codeql-action#62

Open

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/codeql-action#64

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#75

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#76

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 3 vulnerabilities aliscco/codeql-action#83

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#84

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#86

Open

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#88

Open

This was referenced Apr 24, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#90

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#92

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#95

Open

This was referenced Apr 25, 2023

[Snyk] Fix for 3 vulnerabilities aliscco/codeql-action#103

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#104

Open

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#107

Open

	// And the same config should be returned again
	// And that same newly-initialised config should now be returned by getConfig

	let configFile = core.getInput('config-file');
	const configFile = core.getInput('config-file');

	// COPYRIGHT file existed in df4c6869212341b601005567381944ed90906b6b but not in master
	// COPYRIGHT file existed in df4c6869212341b601005567381944ed90906b6b but not in main

	* Checkout a repository at the given ref, and return the directory of the checkout.
	* Check out repository at the given ref, and return the directory of the checkout.

Move config parsing earlier + add to codeql search path #109

Move config parsing earlier + add to codeql search path #109

Uh oh!

Conversation

robertbrignull commented Jul 13, 2020

Merge / deployment checklist

Uh oh!

sampart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants