feat(cli-repl): add ability to set log location MONGOSH-1983 #2326
Conversation
gagik
changed the title
gagik
force-pushed
the
gagik/add-log-location
branch
from
10cb29c to
718990c
Compare
gagik
force-pushed
the
gagik/add-disable-logging
branch
6 times, most recently
from
3b38cc7 to
ef79cf4
Compare
Some tests still need to be fixed
…initialization I think this refactor provides much better readability and provides an easier interface for us to test and debug the functionality of this. This also moves the hookLoggers call to the point of initialization as
Adds validation for the disableLogging field and fixes the equivalent test.
gagik
force-pushed
the
gagik/add-disable-logging
branch
from
054080c to
5147039
Compare
gagik
force-pushed
the
gagik/add-log-location
branch
4 times, most recently
from
3060ce3 to
fa404ed
Compare
gagik
changed the title
gagik
force-pushed
the
gagik/add-log-location
branch
from
fa404ed to
b17d60c
Compare
| @@ -574,6 +575,11 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { | |||
| return `${key} must be a valid URL or empty`; | |||
| } | |||
| return null; | |||
| case 'logLocation': | |||
| if (value !== undefined && typeof value !== 'string') { | |||
There was a problem hiding this comment.
I am wondering if there's a good way to check for valid path. I was thinking regex but worried it'd be end up disallowing valid paths in weirder OSes. Running fs.access could be a way, just seems to be a bit more proactive validator than others.
There was a problem hiding this comment.
path.isAbsolute()? Allowing a relative path seems like a great way to cause chaos, if we do that, we should also specify a relatively fixed path that it is relative to (e.g. os.homedir()).
There was a problem hiding this comment.
This should come with an e2e test that verifies that the file actually gets created and written to (in this section:
mongosh/packages/e2e-tests/test/e2e.spec.ts
Line 1327 in 7e35e10
| @@ -190,7 +190,8 @@ export class CliRepl implements MongoshIOProvider { | |||
| }); | |||
|
|
|||
| this.logManager = new MongoLogManager({ | |||
| directory: this.shellHomeDirectory.localPath('.'), | |||
| directory: | |||
| this.getConfig('logLocation') ?? this.shellHomeDirectory.localPath('.'), | |||
There was a problem hiding this comment.
.getConfig() doesn't work yet before the .generateOrReadConfig() and .loadGlobalConfigFile() calls in .start().
| @@ -574,6 +575,11 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { | |||
| return `${key} must be a valid URL or empty`; | |||
| } | |||
| return null; | |||
| case 'logLocation': | |||
| if (value !== undefined && typeof value !== 'string') { | |||
There was a problem hiding this comment.
path.isAbsolute()? Allowing a relative path seems like a great way to cause chaos, if we do that, we should also specify a relatively fixed path that it is relative to (e.g. os.homedir()).
gagik
force-pushed
the
gagik/add-disable-logging
branch
2 times, most recently
from
275b7ad to
214ca17
Compare
Adds ability to configure log location.