Specify custom directory for storing evaluation logs by shati-patel · Pull Request #863 · github/vscode-codeql

shati-patel · 2021-05-14T17:09:28Z

Will fix #820.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.
- Docs-wise, we should probably mention this in https://un5kwke0ketx6vwhy3c87d8.julianrbryant.com/docs/codeql-for-visual-studio-code/troubleshooting-codeql-for-visual-studio-code/. I'll open a PR for this: Docs: Describe custom log directory setting in VS Code extension codeql#5946

aeisenberg · 2021-05-20T14:33:47Z

extensions/ql-vscode/src/queryserver-client.ts

+      if (fs.existsSync(this.config.customLogDirectory) && fs.statSync(this.config.customLogDirectory).isDirectory()) {
+        storagePath = this.config.customLogDirectory;
+        isCustomLogDirectory = true;
+      } else if (this.config.customLogDirectory) {
+        helpers.showAndLogErrorMessage(`${this.config.customLogDirectory} is not a valid directory. Logs will be stored in a temporary workspace directory instead.`);
+      }


If the custom path doesn't exist, maybe you can create it (with an info message). I think it's only an error if it already exists, but isn't a file.

I made an attempt in b7897b0, which seems to do the right thing, but any tips on syntax/fixes are very welcome 😄

adityasharad · 2021-05-20T21:28:29Z

Looks good so far. Next step is to look at the tests for logging: you'll need to update the tests that use loggers to pass in the new boolean flags you introduced, and check their behaviour in each case.

shati-patel · 2021-05-21T15:04:15Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


I couldn't work out why this test passes with "length equal to 1". Can someone explain? I might be misunderstanding what it's doing... 👀

Is it possible fs.remove is not working because the directory is not empty? Try setting a breakpoint on line 129 and step through to see what happens and what is deleted.

Also, I wonder if that method isn't completely synchronous. You can try addinga a waitABit call in between and see what happens.

Oh the sync/async might be it. I was also baffled by why the existing test worked.

extensions/ql-vscode/src/logging.ts

aeisenberg · 2021-05-21T15:46:40Z

extensions/ql-vscode/src/queryserver-client.ts

+    let isCustomLogDirectory = false;
+    if (this.config.customLogDirectory) {
+      try {
+        fs.mkdirSync(this.config.customLogDirectory);


This method needs to be async

Suggested change

fs.mkdirSync(this.config.customLogDirectory);

await fs.mkdir(this.config.customLogDirectory);

extensions/ql-vscode/src/queryserver-client.ts

aeisenberg · 2021-05-21T16:01:15Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


Is it possible fs.remove is not working because the directory is not empty? Try setting a breakpoint on line 129 and step through to see what happens and what is deleted.

aeisenberg · 2021-05-21T16:02:26Z

extensions/ql-vscode/test/pure-tests/logging.test.ts

+  it('should delete an existing folder when setting the log storage path', async () => {
    fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.init(tempFolders.storagePath.name);
+    logger.setLogStoragePath(tempFolders.storagePath.name, false);
    // should be empty dir

+    const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
+    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
+    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+  });


Also, I wonder if that method isn't completely synchronous. You can try addinga a waitABit call in between and see what happens.

aeisenberg · 2021-05-21T16:09:17Z

extensions/ql-vscode/src/logging.ts

-    fs.remove(this.additionalLogLocationPath);
+    this.isCustomLogDirectory = isCustomLogDirectory;
+
+    if (!this.isCustomLogDirectory) {


So, what this means is whenever someone changes the query server logs from the default to a custom location, then the old logs are deleted. And if they switch back, there won't be any logs at all. Maybe that's ok.

extensions/ql-vscode/src/queryserver-client.ts

aeisenberg

One last change and then .

extensions/ql-vscode/src/queryserver-client.ts

(I haven't got the error handling to work asynchronously, so I stuck with `mkdirSync` for now)

aeisenberg

Great job!

aeisenberg reviewed May 20, 2021

View reviewed changes

shati-patel changed the title ~~[WIP] Specify custom directory for storing evaluation logs~~ Specify custom directory for storing evaluation logs May 21, 2021

shati-patel force-pushed the shati-patel/store-logs branch 2 times, most recently from 892a125 to cd39ede Compare May 21, 2021 15:01

shati-patel marked this pull request as ready for review May 21, 2021 15:03

shati-patel requested review from adityasharad and aeisenberg May 21, 2021 15:03

shati-patel commented May 21, 2021

View reviewed changes

aeisenberg reviewed May 21, 2021

View reviewed changes

shati-patel commented May 21, 2021

View reviewed changes

extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved

shati-patel force-pushed the shati-patel/store-logs branch from 8d711c6 to 8e4930c Compare May 21, 2021 19:55

aeisenberg reviewed May 21, 2021

View reviewed changes

extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved

shati-patel added 5 commits May 21, 2021 21:32

Specify custom directory for storing query server logs

18fc98f

Create custom log directory, if possible

25a6965

(I haven't got the error handling to work asynchronously, so I stuck with `mkdirSync` for now)

Update tests

ad0fb75

Add changelog note

0c03195

Make functions async + other review comments

43fba35

shati-patel force-pushed the shati-patel/store-logs branch from 1637a22 to 43fba35 Compare May 21, 2021 20:34

aeisenberg approved these changes May 21, 2021

View reviewed changes

shati-patel merged commit 2f3be92 into main May 21, 2021

shati-patel deleted the shati-patel/store-logs branch May 21, 2021 20:41

shati-patel mentioned this pull request May 21, 2021

Docs: Describe custom log directory setting in VS Code extension github/codeql#5946

Merged

	fs.mkdirSync(this.config.customLogDirectory);
	await fs.mkdir(this.config.customLogDirectory);

Conversation

shati-patel commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shati-patel May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityasharad commented May 20, 2021

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shati-patel commented May 14, 2021 •

edited

Loading

shati-patel May 20, 2021 •

edited

Loading