From 6d1dce27f008744a87a4c53d49e76ea941abcef2 Mon Sep 17 00:00:00 2001 From: rayangler <27821750+rayangler@users.noreply.github.com> Date: Thu, 28 Sep 2023 16:19:39 -0400 Subject: [PATCH] DOP-3723: Use Autobuilder job id as persistence module build id (#913) --- modules/persistence/index.ts | 10 ++-- .../persistence/src/services/pages/index.ts | 21 ++++++-- .../persistence/tests/services/pages.test.ts | 48 ++++++++++++------- src/job/productionJobHandler.ts | 2 +- src/job/stagingJobHandler.ts | 4 +- tests/data/data.ts | 4 +- 6 files changed, 60 insertions(+), 29 deletions(-) diff --git a/modules/persistence/index.ts b/modules/persistence/index.ts index 5fbf6d143..433be411c 100644 --- 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 index 38addc05a..81dd98d5d 100644 --- a/modules/persistence/src/services/pages/index.ts +++ b/modules/persistence/src/services/pages/index.ts @@ -109,13 +109,21 @@ class UpdatedPagesManager { prevPageIds: Set; updateTime: Date; githubUser: string; - - constructor(prevPageDocsMapping: PreviousPageMapping, prevPagesIds: Set, pages: Page[], githubUser: string) { + buildId: ObjectId; + + constructor( + prevPageDocsMapping: PreviousPageMapping, + prevPagesIds: Set, + 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 index 24161f2d7..03b059f2f 100644 --- 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(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,9 +140,15 @@ 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(); @@ -143,7 +156,7 @@ describe('pages module', () => { 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(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(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(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(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(collection).findOne(findQuery); diff --git a/src/job/productionJobHandler.ts b/src/job/productionJobHandler.ts index bbe3ce770..1c933f1a3 100644 --- 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 index 0f9680c8d..919443b04 100644 --- 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 index 80aac8875..396884a7f 100644 --- a/tests/data/data.ts +++ b/tests/data/data.ts @@ -145,7 +145,7 @@ export class TestDataProvider { return Array().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().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 : '';