From 3f40d5932fda26ab3695908711968c932de06a6a Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 22:26:32 +0100 Subject: [PATCH 1/5] wip: check what path sep is when using windows runner --- packages/types/src/index.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7dcc2c9dbf..4eb2331edb 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -586,6 +586,13 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { + // eslint-disable-next-line no-console + console.info( + path.sep, + path.win32 === path, + path.isAbsolute(value as string), + value + ); return `${key} must be a valid absolute path or empty`; } return null; From 23f84d81120e3ca88334476f98c069adb4605f5e Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 11:58:51 +0100 Subject: [PATCH 2/5] Revert "wip: check what path sep is when using windows runner" This reverts commit 85453b9bf32bbdd3d02f62b90ece16559749f7fb. --- packages/types/src/index.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 4eb2331edb..7dcc2c9dbf 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -586,13 +586,6 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { - // eslint-disable-next-line no-console - console.info( - path.sep, - path.win32 === path, - path.isAbsolute(value as string), - value - ); return `${key} must be a valid absolute path or empty`; } return null; From dbe5b984a48f077d20e48d66e8dec8806dc7782a Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 10:51:51 +0100 Subject: [PATCH 3/5] feat(cli-repl): add ability to set log retention days MONGOSH-1984 --- packages/cli-repl/src/cli-repl.spec.ts | 14 ++++++++++++++ packages/cli-repl/src/cli-repl.ts | 2 +- packages/types/src/index.spec.ts | 6 ++++++ packages/types/src/index.ts | 2 ++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 80321aeeed..c89c61a8cc 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -323,6 +323,7 @@ describe('CliRepl', function () { 'updateURL', 'disableLogging', 'logLocation', + 'logRetentionDays', ] satisfies (keyof CliUserConfig)[]); }); @@ -1443,6 +1444,19 @@ describe('CliRepl', function () { ) ); }); + + it('can set log retention days', async function () { + const testRetentionDays = 123; + cliRepl.config.logRetentionDays = testRetentionDays; + await cliRepl.start(await testServer.connectionString(), {}); + + expect(cliRepl.getConfig('logRetentionDays')).equals( + testRetentionDays + ); + expect(cliRepl.logManager?._options.retentionDays).equals( + testRetentionDays + ); + }); }); it('times out fast', async function () { diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts index 908fbe5b04..88e05a4284 100644 --- a/packages/cli-repl/src/cli-repl.ts +++ b/packages/cli-repl/src/cli-repl.ts @@ -259,7 +259,7 @@ export class CliRepl implements MongoshIOProvider { directory: (await this.getConfig('logLocation')) || this.shellHomeDirectory.localPath('.'), - retentionDays: 30, + retentionDays: await this.getConfig('logRetentionDays'), maxLogFileCount: +( process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100 ), diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index 02c5dccc04..1393129358 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -25,6 +25,12 @@ describe('config validation', function () { expect(await validate('historyLength', 0)).to.equal(null); expect(await validate('historyLength', 1)).to.equal(null); expect(await validate('historyLength', Infinity)).to.equal(null); + expect(await validate('logRetentionDays', 'foo')).to.equal( + 'logRetentionDays must be a positive integer' + ); + expect(await validate('logRetentionDays', -1)).to.equal( + 'logRetentionDays must be a positive integer' + ); expect(await validate('showStackTraces', 'foo')).to.equal( 'showStackTraces must be a boolean' ); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7dcc2c9dbf..bdf495f93a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -509,6 +509,7 @@ export class CliUserConfig extends SnippetShellUserConfig { updateURL = 'https://downloads.mongodb.com/compass/mongosh.json'; disableLogging = false; logLocation: string | undefined = undefined; + logRetentionDays = 30; } export class CliUserConfigValidator extends SnippetShellUserConfigValidator { @@ -531,6 +532,7 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { return null; case 'inspectDepth': case 'historyLength': + case 'logRetentionDays': if (typeof value !== 'number' || value < 0) { return `${key} must be a positive integer`; } From 141ae656715a03293fc72e1b0d43859628d8af6c Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 14:43:38 +0100 Subject: [PATCH 4/5] tests: add file deletion e2e tests --- packages/e2e-tests/test/e2e.spec.ts | 71 ++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 08f6a56e7b..a7e9a114a0 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1,7 +1,7 @@ /* eslint-disable no-control-regex */ import { expect } from 'chai'; import type { Db } from 'mongodb'; -import { MongoClient } from 'mongodb'; +import { MongoClient, ObjectId } from 'mongodb'; import { eventually } from '../../../testing/eventually'; import { TestShell } from './test-shell'; @@ -1676,6 +1676,75 @@ describe('e2e', function () { }); }); + /** Helper to visualize and compare the existence of files in a specific order. + * Returns a string comprised of: 1 if a given file exists, 0 otherwise. */ + const getFilesState = async (paths: string[]) => { + return ( + await Promise.all( + paths.map((path) => + fs.stat(path).then( + () => 1, + () => 0 + ) + ) + ) + ).join(''); + }; + + describe('with custom log retention days', function () { + const customLogDir = useTmpdir(); + + it('should delete older files according to the setting', async function () { + const paths: string[] = []; + const today = Math.floor(Date.now() / 1000); + const tenDaysAgo = today - 10 * 24 * 60 * 60; + // Create 6 files older than 7 days + for (let i = 5; i >= 0; i--) { + const filename = path.join( + customLogDir.path, + ObjectId.createFromTime(tenDaysAgo - i).toHexString() + '_log' + ); + await fs.writeFile(filename, ''); + paths.push(filename); + } + // Create 4 files newer than 10 days + for (let i = 3; i >= 0; i--) { + const filename = path.join( + customLogDir.path, + ObjectId.createFromTime(today - i).toHexString() + '_log' + ); + await fs.writeFile(filename, ''); + paths.push(filename); + } + + const retentionDays = 7; + + const globalConfig = path.join(homedir, 'globalconfig.conf'); + await fs.writeFile( + globalConfig, + `mongosh:\n logLocation: "${customLogDir.path}"\n logRetentionDays: ${retentionDays}` + ); + + expect(await getFilesState(paths)).equals('1111111111'); + + shell = this.startTestShell({ + args: ['--nodb'], + env: { + ...env, + MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig, + }, + forceTerminal: true, + }); + + await shell.waitForPrompt(); + + // Add the newly created log file + paths.push(path.join(customLogDir.path, `${shell.logId}_log`)); + // Expect 6 files to be deleted and 5 to remain (including the new log file) + expect(await getFilesState(paths)).equals('00000011111'); + }); + }); + it('creates a log file that keeps track of session events', async function () { expect(await shell.executeLine('print(123 + 456)')).to.include('579'); const log = await readLogFile(); From 90dbd8f86a75a8f4efb26f8d79ad4fc2a5d16d6a Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 14:52:42 +0100 Subject: [PATCH 5/5] fix(tests): use a promise for getting config --- packages/cli-repl/src/cli-repl.spec.ts | 2 +- packages/e2e-tests/test/e2e.spec.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index c89c61a8cc..b9af6f9bd4 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1450,7 +1450,7 @@ describe('CliRepl', function () { cliRepl.config.logRetentionDays = testRetentionDays; await cliRepl.start(await testServer.connectionString(), {}); - expect(cliRepl.getConfig('logRetentionDays')).equals( + expect(await cliRepl.getConfig('logRetentionDays')).equals( testRetentionDays ); expect(cliRepl.logManager?._options.retentionDays).equals( diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index a7e9a114a0..93c8cc373f 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1722,7 +1722,9 @@ describe('e2e', function () { const globalConfig = path.join(homedir, 'globalconfig.conf'); await fs.writeFile( globalConfig, - `mongosh:\n logLocation: "${customLogDir.path}"\n logRetentionDays: ${retentionDays}` + `mongosh:\n logLocation: ${JSON.stringify( + customLogDir.path + )}\n logRetentionDays: ${retentionDays}` ); expect(await getFilesState(paths)).equals('1111111111');