Skip to content

JS: Skip CodeQL databases in AutoBuild#2258

Merged
semmle-qlci merged 2 commits intogithub:masterfrom
asger-semmle:js-ignore-codesql-databases
Nov 14, 2019
Merged

JS: Skip CodeQL databases in AutoBuild#2258
semmle-qlci merged 2 commits intogithub:masterfrom
asger-semmle:js-ignore-codesql-databases

Conversation

@asger-semmle
Copy link
Contributor

No description provided.

@asger-semmle asger-semmle added the JS label Nov 5, 2019
@asger-semmle asger-semmle requested a review from a team as a code owner November 5, 2019 16:34
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

LGTM, modulo potential incompleteness.

throws IOException {
if (!dir.equals(currentRoot[0]) && (excludes.contains(dir) || dir.toFile().isHidden()))
return FileVisitResult.SKIP_SUBTREE;
if (dir.resolve("codeql-database.yml").toFile().exists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we checked on #codeql-cli if that is the only new thing we should ignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

This ought to suffice for excluding databases made by the new CLI.

It won't take if you trick Odasa into creating a snapshot in a subdirectory of the source tree, but I believe doing so would takes quite a bit more coercion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps qlpack.yml, to avoid extracting test cases in existing qlpacks?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be problematic for extracting a test directory that has its own qlpack.yml.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, why ...toFile().exists() rather than Files.exists(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old habit I suppose. Switching to Files.exists().

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

It sounds like this is good to go.

@semmle-qlci semmle-qlci merged commit 67963a5 into github:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants