DOP-5042: Search-manifest Netlify Integration Environments #38
Conversation
❌ Deploy Preview for redoc-ext-test failed. Why did it fail? →
|
✅ Deploy Preview for persistence-module-ext canceled.
|
✅ Deploy Preview for git-changed-file-extension canceled.
|
✅ Deploy Preview for populate-data-extension canceled.
|
❌ Deploy Preview for site-links-display failed. Why did it fail? →
|
❌ Deploy Preview for search-manifest-integration failed. Why did it fail? →
|
✅ Deploy Preview for search-manifest-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for snooty-cache-extension canceled.
|
❌ Deploy Preview for test-monorepo-redoc failed. Why did it fail? →
|
…dates in build/util
✅ Deploy Preview for redirects-and-publish-extension canceled.
|
Merge in latest from msin, extension improvements
Merge in latest from main
✅ Deploy Preview for slack-deploy-extension canceled.
|
| @@ -83,7 +89,9 @@ export const generateAndUploadManifests = async ({ | |||
| AWS_S3_SECRET_ACCESS_KEY: dbEnvVars.AWS_S3_SECRET_ACCESS_KEY, | |||
| }); | |||
|
|
|||
| console.log(`S3 upload status: ${JSON.stringify(s3Status)}`); | |||
| console.log( | |||
| `S3 upload status: ${JSON.stringify(s3Status.$metadata.httpStatusCode)}`, | |||
There was a problem hiding this comment.
NIT: I'm assuming we don't need the JSON.stringify when logging the httpStatusCode
|
|
||
| for (const entry of mappedEntries) { | ||
| // Read and decode each entry | ||
| const decoded = BSON.deserialize(readFileSync(`documents/${entry}`)); |
There was a problem hiding this comment.
We should use the async version of reading files whenever we can. The one case where you really need to use a sync function is with existsSync. Otherwise, async version of readFile is preferable. This would also you allow you to do a Promise.all(mappedEntries.map(...)) instead of the for loop.
| searchProperty: string, | ||
| connectionInfo: SearchClusterConnectionInfo, | ||
| ) => { | ||
| const documentsColl = await getDocumentsCollection({ ...connectionInfo }); |
There was a problem hiding this comment.
await getDocumentsCollection({ ...connectionInfo }); is equivalent to await getDocumentsCollection(connectionInfo);
| `Error removing stale property ${searchProperty} in database ${connectionInfo.databaseName}, collection ${connectionInfo.collectionName}: ${e}`, | ||
| ); | ||
| } finally { | ||
| closeSearchDb(); |
There was a problem hiding this comment.
I believe we need an await here
anabellabuckvar
changed the title
| const mappedEntries = entries.filter((fileName) => { | ||
| return ( | ||
| fileName.includes('.bson') && | ||
| !fileName.includes('images') && |
There was a problem hiding this comment.
I think we might want to add a forward slash at the end images/ so that we don't accidentally exclude .bson files that might have the word images or includes in them
There was a problem hiding this comment.
Good catch, thank you!!
| ? 'docs-search-indexes-test/preprd' | ||
| : 'docs-search-indexes-test/prd', | ||
| prefix: 'search-indexes/', | ||
| // TODO: make into constants? |
There was a problem hiding this comment.
nit, i think the bucketName docs-search-indexes-test can also be a const FWIW. we upload to a single search bucket and separate envs by path (can leave as a comment!)
extensions/search-manifest/src/uploadToAtlas/getSearchProperties.ts
Outdated
Show resolved
Hide resolved
| const status = await documentsColl?.deleteMany(query); | ||
| return status; | ||
| try { | ||
| const status = await documentsColl?.deleteMany(query); |
There was a problem hiding this comment.
Instead of using optional chaining, I think we should throw an error in the getDocumentsCollection function if we don't have a documentsColl object. If we don't have that, then this code doesn't work, and I think it makes sense to have that error occur where it happens.
There was a problem hiding this comment.
I just removed the optional chaining so an error will be thrown if we try to run deleteMany() on a nonexistent collection
extensions/search-manifest/src/uploadToAtlas/getSearchProperties.ts
Outdated
Show resolved
Hide resolved
TICKET
DOP-5042
Search-manifest Netlify Integration: Environments
NOTES
This PR cleans up the search-manifest extension by updating hardcoded variables to be environment-dependent, using shared code from the
libs/utilspackage, and generally refactoring.Upon deployment of any repo with
/netlify-test-deployin Slack, a manifest will be generated for that repo and uploaded to the dotcomstg S3 bucket and dotcomstg Atlas db for search manifests. The dotcomstg S3 bucket isdocs-search-indexes-test/search-indexes/preprd/The dotcomstg Atlas collection is
search-staging.documentsunder the Search cluster.