feat(cli-repl): add configuration to set max file count MONGOSH-1987 (#…

…2354)
mongodb-js · Feb 10, 2025 · d789f31 · d789f31
1 parent 27a51ae
commit d789f31
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 17 deletions.
diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts
@@ -324,6 +324,7 @@ describe('CliRepl', function () {
           'disableLogging',
           'logLocation',
           'logRetentionDays',
+          'logMaxFileCount',
         ] satisfies (keyof CliUserConfig)[]);
       });
 
@@ -1457,6 +1458,25 @@ describe('CliRepl', function () {
               testRetentionDays
             );
           });
+
+          it('can set log max file count', async function () {
+            const testMaxFileCount = 123;
+            cliRepl.config.logMaxFileCount = testMaxFileCount;
+            const oldEnvironmentLimit =
+              process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT;
+            delete process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT;
+            await cliRepl.start(await testServer.connectionString(), {});
+
+            expect(await cliRepl.getConfig('logMaxFileCount')).equals(
+              testMaxFileCount
+            );
+            expect(cliRepl.logManager?._options.maxLogFileCount).equals(
+              testMaxFileCount
+            );
+
+            process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT =
+              oldEnvironmentLimit;
+          });
         });
 
         it('times out fast', async function () {

diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts
@@ -261,7 +261,8 @@ export class CliRepl implements MongoshIOProvider {
         this.shellHomeDirectory.localPath('.'),
       retentionDays: await this.getConfig('logRetentionDays'),
       maxLogFileCount: +(
-        process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100
+        process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT ||
+        (await this.getConfig('logMaxFileCount'))
       ),
       onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
       onwarn: (err: Error, path: string) =>

diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts
@@ -1747,6 +1747,58 @@ describe('e2e', function () {
           });
         });
 
+        describe('with custom log retention max file count', function () {
+          const customLogDir = useTmpdir();
+
+          it('should delete files once it is above the max file count limit', async function () {
+            const globalConfig = path.join(homedir, 'globalconfig.conf');
+            await fs.writeFile(
+              globalConfig,
+              `mongosh:\n  logLocation: ${JSON.stringify(
+                customLogDir.path
+              )}\n  logMaxFileCount: 4`
+            );
+            const paths: string[] = [];
+            const offset = Math.floor(Date.now() / 1000);
+
+            // Create 10 log files
+            for (let i = 9; i >= 0; i--) {
+              const filename = path.join(
+                customLogDir.path,
+                ObjectId.createFromTime(offset - i).toHexString() + '_log'
+              );
+              await fs.writeFile(filename, '');
+              paths.push(filename);
+            }
+
+            // All 10 existing log files exist.
+            expect(await getFilesState(paths)).to.equal('1111111111');
+            shell = this.startTestShell({
+              args: ['--nodb'],
+              env: {
+                ...env,
+                MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT: '',
+                MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
+              },
+              forceTerminal: true,
+            });
+
+            await shell.waitForPrompt();
+
+            // Add the newly created log to the file list.
+            paths.push(
+              path.join(customLogDir.path, `${shell.logId as string}_log`)
+            );
+
+            expect(
+              await shell.executeLine('config.get("logMaxFileCount")')
+            ).contains('4');
+
+            // Expect 7 files to be deleted and 4 to remain (including the new log file)
+            expect(await getFilesState(paths)).to.equal('00000001111');
+          });
+        });
+
         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();
@@ -1816,23 +1868,26 @@ describe('e2e', function () {
           const newLogId = shell.logId;
           expect(newLogId).not.null;
           expect(oldLogId).equals(newLogId);
-          log = await readLogFile();
 
-          expect(
-            log.filter(
-              (logEntry) => logEntry.attr?.input === 'print(111 + 222)'
-            )
-          ).to.have.lengthOf(1);
-          expect(
-            log.filter(
-              (logEntry) => logEntry.attr?.input === 'print(579 - 123)'
-            )
-          ).to.have.lengthOf(1);
-          expect(
-            log.filter(
-              (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
-            )
-          ).to.have.lengthOf(0);
+          await eventually(async () => {
+            log = await readLogFile();
+
+            expect(
+              log.filter(
+                (logEntry) => logEntry.attr?.input === 'print(111 + 222)'
+              )
+            ).to.have.lengthOf(1);
+            expect(
+              log.filter(
+                (logEntry) => logEntry.attr?.input === 'print(579 - 123)'
+              )
+            ).to.have.lengthOf(1);
+            expect(
+              log.filter(
+                (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
+              )
+            ).to.have.lengthOf(0);
+          });
         });
 
         it('includes information about the driver version', async function () {

diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts
@@ -31,6 +31,12 @@ describe('config validation', function () {
     expect(await validate('logRetentionDays', -1)).to.equal(
       'logRetentionDays must be a positive integer'
     );
+    expect(await validate('logMaxFileCount', 'foo')).to.equal(
+      'logMaxFileCount must be a positive integer'
+    );
+    expect(await validate('logMaxFileCount', -1)).to.equal(
+      'logMaxFileCount 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
@@ -510,6 +510,7 @@ export class CliUserConfig extends SnippetShellUserConfig {
   disableLogging = false;
   logLocation: string | undefined = undefined;
   logRetentionDays = 30;
+  logMaxFileCount = 100;
 }
 
 export class CliUserConfigValidator extends SnippetShellUserConfigValidator {
@@ -533,6 +534,7 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator {
       case 'inspectDepth':
       case 'historyLength':
       case 'logRetentionDays':
+      case 'logMaxFileCount':
         if (typeof value !== 'number' || value < 0) {
           return `${key} must be a positive integer`;
         }