DOP-3723: Use Autobuilder job id as persistence module build id (#913)

mongodb · Sep 28, 2023 · 6d1dce2 · 6d1dce2
1 parent 0e3a974
commit 6d1dce2
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 29 deletions.
diff --git a/modules/persistence/index.ts b/modules/persistence/index.ts
@@ -18,6 +18,7 @@ import { upsertAssets } from './src/services/assets';
 interface ModuleArgs {
   path: string;
   githubUser: string;
+  jobId: string;
   strict: string;
   [props: string | number | symbol]: unknown;
 }
@@ -30,14 +31,17 @@ const missingPathMessage = 'No path specified in arguments - please specify a bu
 // Load command line args into a parameterized argv
 const argv: ModuleArgs = minimist(process.argv.slice(2));
 
-const app = async (path: string, githubUser: string) => {
+const app = async (path: string, githubUser: string, jobId: string) => {
   try {
     if (!path) throw missingPathMessage;
     const user = githubUser || 'docs-builder-bot';
     const zip = new AdmZip(path);
+
+    // Safely convert jobId in case of empty string
+    const autobuilderJobId = jobId || undefined;
     // atomic buildId for all artifacts read by this module - fundamental assumption
     // that only one build will be used per run of this module.
-    const buildId = new mongodb.ObjectId();
+    const buildId = new mongodb.ObjectId(autobuilderJobId);
     const metadata = await metadataFromZip(zip, user);
     // initialize db connections to handle shared connections
     await snootyDb();
@@ -55,7 +59,7 @@ const app = async (path: string, githubUser: string) => {
   }
 };
 
-app(argv['path'], argv['githubUser']).catch(() => {
+app(argv['path'], argv['githubUser'], argv['jobId']).catch(() => {
   console.error('Persistence Module Failure. Ending build.');
   process.exit(1);
 });
diff --git a/modules/persistence/src/services/pages/index.ts b/modules/persistence/src/services/pages/index.ts
@@ -109,13 +109,21 @@ class UpdatedPagesManager {
   prevPageIds: Set<string>;
   updateTime: Date;
   githubUser: string;
-
-  constructor(prevPageDocsMapping: PreviousPageMapping, prevPagesIds: Set<string>, pages: Page[], githubUser: string) {
+  buildId: ObjectId;
+
+  constructor(
+    prevPageDocsMapping: PreviousPageMapping,
+    prevPagesIds: Set<string>,
+    pages: Page[],
+    githubUser: string,
+    buildId: ObjectId
+  ) {
     this.currentPages = pages;
     this.operations = [];
     this.prevPageDocsMapping = prevPageDocsMapping;
     this.prevPageIds = prevPagesIds;
     this.githubUser = githubUser;
+    this.buildId = buildId;
 
     this.updateTime = new Date();
     this.checkForPageDiffs();
@@ -152,6 +160,8 @@ class UpdatedPagesManager {
                 static_assets: this.findUpdatedAssets(page.static_assets, prevPageData?.static_assets),
                 updated_at: this.updateTime,
                 deleted: false,
+                // Track the last build ID to update the content
+                build_id: this.buildId,
               },
               $setOnInsert: {
                 created_at: this.updateTime,
@@ -226,6 +236,7 @@ class UpdatedPagesManager {
             $set: {
               deleted: true,
               updated_at: this.updateTime,
+              build_id: this.buildId,
             },
           },
         },
@@ -247,7 +258,7 @@ class UpdatedPagesManager {
  * @param pages
  * @param collection
  */
-const updatePages = async (pages: Page[], collection: string, githubUser: string) => {
+const updatePages = async (pages: Page[], collection: string, githubUser: string, buildId: ObjectId) => {
   if (pages.length === 0) {
     return;
   }
@@ -264,7 +275,7 @@ const updatePages = async (pages: Page[], collection: string, githubUser: string
 
     const diffsTimerLabel = 'finding page differences';
     console.time(diffsTimerLabel);
-    const updatedPagesManager = new UpdatedPagesManager(prevPageDocsMapping, prevPageIds, pages, githubUser);
+    const updatedPagesManager = new UpdatedPagesManager(prevPageDocsMapping, prevPageIds, pages, githubUser, buildId);
     const operations = updatedPagesManager.getOperations();
     console.timeEnd(diffsTimerLabel);
 
@@ -293,7 +304,7 @@ export const insertAndUpdatePages = async (buildId: ObjectId, zip: AdmZip, githu
 
     const featureEnabled = process.env.FEATURE_FLAG_UPDATE_PAGES;
     if (featureEnabled && featureEnabled.toUpperCase() === 'TRUE') {
-      ops.push(updatePages(pages, UPDATED_AST_COLL_NAME, githubUser));
+      ops.push(updatePages(pages, UPDATED_AST_COLL_NAME, githubUser, buildId));
     }
 
     return Promise.all(ops);

diff --git a/modules/persistence/tests/services/pages.test.ts b/modules/persistence/tests/services/pages.test.ts
@@ -3,6 +3,8 @@ import { Page, UpdatedPage, _updatePages } from '../../src/services/pages';
 import { closeDb, setMockDB } from '../utils';
 import { ObjectID } from 'bson';
 
+const GH_USER = 'foo_user';
+
 // Ignore non-null assertion warnings for this file. Non-null assertions are most likely
 // to be used for responses that we guarantee are not null
 /* eslint-disable  @typescript-eslint/no-non-null-assertion */
@@ -47,12 +49,14 @@ describe('pages module', () => {
           filename: 'page0.txt',
           ast: { foo: 'foo', bar: { foo: 'foo' } },
           static_assets: [],
+          github_username: GH_USER,
         },
         {
           page_id: `${pagePrefix}/page1.txt`,
           filename: 'page1.txt',
           ast: { foo: 'foo', bar: { foo: 'bar' } },
           static_assets: [],
+          github_username: GH_USER,
         },
       ];
     };
@@ -72,9 +76,10 @@ describe('pages module', () => {
         filename: 'includes/included-file.rst',
         ast: { foo: 'foo', bar: { foo: 'foo' } },
         static_assets: [],
+        github_username: GH_USER,
       };
       pages.push(rstFile);
-      await _updatePages(pages, collection);
+      await _updatePages(pages, collection, GH_USER, new ObjectID());
 
       res = await mockDb.collection(collection).find(findQuery).toArray();
       expect(res).toHaveLength(2);
@@ -85,7 +90,7 @@ describe('pages module', () => {
     it('should update modified pages', async () => {
       const pagePrefix = generatePagePrefix();
       const pages = generatePages(pagePrefix);
-      await _updatePages(pages, collection);
+      await _updatePages(pages, collection, GH_USER, new ObjectID());
 
       const findQuery = {
         page_id: { $regex: new RegExp(`^${pagePrefix}`) },
@@ -102,15 +107,17 @@ describe('pages module', () => {
           filename: 'page0.txt',
           ast: { foo: 'foo', bar: { foo: 'foo' } },
           static_assets: [],
+          github_username: GH_USER,
         },
         {
           page_id: `${pagePrefix}/page1.txt`,
           filename: 'page1.txt',
           ast: { foo: 'foo', bar: { foo: 'baz' } },
           static_assets: [],
+          github_username: GH_USER,
         },
       ];
-      await _updatePages(updatedPages, collection);
+      await _updatePages(updatedPages, collection, GH_USER, new ObjectID());
 
       res = await mockDb.collection<UpdatedPage>(collection).find(findQuery).toArray();
       expect(res).toHaveLength(2);
@@ -123,7 +130,7 @@ describe('pages module', () => {
     it('should mark pages for deletion', async () => {
       const pagePrefix = generatePagePrefix();
       const pages = generatePages(pagePrefix);
-      await _updatePages(pages, collection);
+      await _updatePages(pages, collection, GH_USER, new ObjectID());
 
       const findQuery = {
         page_id: { $regex: new RegExp(`^${pagePrefix}`) },
@@ -133,17 +140,23 @@ describe('pages module', () => {
       expect(res).toHaveLength(0);
 
       const updatedPages = [
-        { page_id: `${pagePrefix}/page1.txt`, filename: 'page1.txt', ast: { foo: 'foo', bar: { foo: 'bar' } } },
+        {
+          page_id: `${pagePrefix}/page1.txt`,
+          filename: 'page1.txt',
+          ast: { foo: 'foo', bar: { foo: 'bar' } },
+          static_assets: [],
+          github_username: GH_USER,
+        },
       ];
-      await _updatePages(updatedPages, collection);
+      await _updatePages(updatedPages, collection, GH_USER, new ObjectID());
 
       // There should be 1 page marked as deleted
       res = await mockDb.collection(collection).find(findQuery).toArray();
       expect(res).toHaveLength(1);
       expect(res[0]).toHaveProperty('filename', 'page0.txt');
 
       // Re-adding the deleted page should lead to no deleted pages
-      await _updatePages(pages, collection);
+      await _updatePages(pages, collection, GH_USER, new ObjectID());
       res = await mockDb.collection(collection).find(findQuery).toArray();
       expect(res).toHaveLength(0);
     });
@@ -160,6 +173,7 @@ describe('pages module', () => {
           filename: 'page0.txt',
           ast: { foo: 'foo', bar: { foo: 'foo' } },
           static_assets: [],
+          github_username: GH_USER,
         };
 
         if (withAssets) {
@@ -173,7 +187,7 @@ describe('pages module', () => {
         // Setup for empty static assets
         const pagePrefix = generatePagePrefix();
         const page = createSamplePage(pagePrefix);
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         const findQuery = { page_id: page.page_id };
         let res = await mockDb.collection<UpdatedPage>(collection).findOne(findQuery);
@@ -182,7 +196,7 @@ describe('pages module', () => {
 
         // Simulate update in page
         page.ast.foo = 'foobar';
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
         res = await mockDb.collection<UpdatedPage>(collection).findOne(findQuery);
         expect(res).toBeTruthy();
         // Should still be 0
@@ -193,13 +207,13 @@ describe('pages module', () => {
         // Setup for empty static assets
         const pagePrefix = generatePagePrefix();
         const page = createSamplePage(pagePrefix);
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Modify page with new AST; a change in static_assets implies a change in AST
         page.ast.foo = 'new assets';
         page.static_assets = sampleStaticAssets;
         const numStaticAssets = page.static_assets.length;
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Check that both assets were added
         const findQuery = { page_id: page.page_id };
@@ -213,7 +227,7 @@ describe('pages module', () => {
       it('should keep assets the same when no assets are changed', async () => {
         const pagePrefix = generatePagePrefix();
         const page = createSamplePage(pagePrefix, true);
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Check that static assets were saved
         const findQuery = { page_id: page.page_id };
@@ -223,7 +237,7 @@ describe('pages module', () => {
 
         // Simulate change in AST but not in static assets
         page.ast.foo = 'no change in assets';
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Check to make sure no changes in static assets
         res = await mockDb.collection<UpdatedPage>(collection).findOne(findQuery);
@@ -236,7 +250,7 @@ describe('pages module', () => {
         const page = createSamplePage(pagePrefix, true);
         const originalKey = page.static_assets[1].key;
         const numStaticAssets = page.static_assets.length;
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Check to make sure asset we plan to change was successfully added
         const findQuery = { page_id: page.page_id };
@@ -249,7 +263,7 @@ describe('pages module', () => {
         page.ast.foo = 'change in one asset';
         const changedKey = '/images/changed-asset-name.svg';
         page.static_assets[1].key = changedKey;
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         // Make sure changed asset is different from original asset
         res = await mockDb.collection<UpdatedPage>(collection).findOne(findQuery);
@@ -267,11 +281,11 @@ describe('pages module', () => {
         // Setup for single static asset
         const pagePrefix = generatePagePrefix();
         const page = createSamplePage(pagePrefix, true);
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         page.ast.foo = 'deleted assets';
         page.static_assets = [];
-        await _updatePages([page], collection);
+        await _updatePages([page], collection, GH_USER, new ObjectID());
 
         const findQuery = { page_id: page.page_id };
         const res = await mockDb.collection<UpdatedPage>(collection).findOne(findQuery);

diff --git a/src/job/productionJobHandler.ts b/src/job/productionJobHandler.ts
@@ -137,7 +137,7 @@ export class ProductionJobHandler extends JobHandler {
     if (this.currJob?.buildCommands) {
       this.currJob.buildCommands[this.currJob.buildCommands.length - 1] = 'make get-build-dependencies';
       this.currJob.buildCommands.push('make next-gen-parse');
-      this.currJob.buildCommands.push('make persistence-module');
+      this.currJob.buildCommands.push(`make persistence-module JOB_ID=${this.currJob._id}`);
       this.currJob.buildCommands.push('make next-gen-html');
       this.currJob.buildCommands.push(`make oas-page-build MUT_PREFIX=${this.currJob.payload.mutPrefix}`);
     }

diff --git a/src/job/stagingJobHandler.ts b/src/job/stagingJobHandler.ts
@@ -61,7 +61,9 @@ export class StagingJobHandler extends JobHandler {
   prepStageSpecificNextGenCommands(): void {
     if (this.currJob.buildCommands) {
       this.currJob.buildCommands[this.currJob.buildCommands.length - 1] = 'make next-gen-parse';
-      this.currJob.buildCommands.push(`make persistence-module GH_USER=${this.currJob.payload.repoOwner}`);
+      this.currJob.buildCommands.push(
+        `make persistence-module GH_USER=${this.currJob.payload.repoOwner} JOB_ID=${this.currJob._id}`
+      );
       this.currJob.buildCommands.push('make next-gen-html');
       const project = this.currJob.payload.project === 'cloud-docs' ? this.currJob.payload.project : '';
       const branchName = /^[a-zA-Z0-9_\-\./]+$/.test(this.currJob.payload.branchName)

diff --git a/tests/data/data.ts b/tests/data/data.ts
@@ -145,7 +145,7 @@ export class TestDataProvider {
     return Array<string>().concat(genericCommands.slice(0, genericCommands.length - 1), [
       'make get-build-dependencies',
       'make next-gen-parse',
-      'make persistence-module',
+      `make persistence-module JOB_ID=${job._id}`,
       'make next-gen-html',
       `make oas-page-build MUT_PREFIX=${job.payload.mutPrefix}`,
     ]);
@@ -163,7 +163,7 @@ export class TestDataProvider {
     const genericCommands = TestDataProvider.getCommonBuildCommands(job);
     const commands = Array<string>().concat(genericCommands.slice(0, genericCommands.length - 1), [
       'make next-gen-parse',
-      `make persistence-module GH_USER=${job.payload.repoOwner}`,
+      `make persistence-module GH_USER=${job.payload.repoOwner} JOB_ID=${job._id}`,
       `make next-gen-html`,
     ]);
     const project = job.payload.project === 'cloud-docs' ? job.payload.project : '';