Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📦 DOP-4497 updtes the mongodb node driver #1035

Merged
merged 13 commits into from
May 7, 2024
Merged

📦 DOP-4497 updtes the mongodb node driver #1035

merged 13 commits into from
May 7, 2024

Conversation

caesarbell
Copy link
Collaborator

@caesarbell caesarbell commented May 2, 2024

Stories/Links:

DOP-4497

Notes

This PR was focused on updating MongoDB Node Driver fromv5.1.0 to v6.5. With upgrading the version, an issue occurred where the build would hang.

Upon running the AB locally, I was able to discover that value was not a property of the response, which lead me to remove the value proeprty and return just the response.

After the above change, I was able to successfully trigger the AB build using the webhook, resulting in the following master and DOP-4497 build results.

The page can be viewed here.

Update
After some additional research, the reason for this change was because in 6.0+ sets includeResultMetadata to false by default. Adding includeResultMetadata true as an option would allow us to return value which would contain our returned document.

Resource Links

Behavioral Changes to Find One Family APIs
Server Release Compatibility Changes

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

Copy link

github-actions bot commented May 2, 2024

Your feature branch infrastructure has been deployed!

Your webhook URL is: https://jxt5lhic6i.execute-api.us-east-2.amazonaws.com/prod/webhook/githubEndpoint/trigger/build

For more information on how to use this endpoint, follow these instructions.

@caesarbell caesarbell requested review from mmeigs and seungpark May 3, 2024 20:22
@caesarbell caesarbell marked this pull request as ready for review May 3, 2024 20:53
Copy link
Contributor

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this makes me a teeny bit nervous because this change could/is more wide-ranging.

  1. Do we know where in breaking changes for the Node driver this occurs? lol @ searching our own documentation
  2. This (the value property) definitely is accessed other places.
  • one example: in the same file (src/repositories/jobRepository) in the function updateWithStatus.

Caesar Bell added 3 commits May 6, 2024 12:31
@caesarbell
Copy link
Collaborator Author

For test sake, I applied the following option {includeResultMetadata: true}, to

async updateWithStatus(
    id: string | mongodb.ObjectId,
    result: any,
    status: JobStatus,
    shouldNotifySqs = true
  ):

to allow the value to be returned, since removing value from updateWithStatus method causes the test to fail with some odd behavior.

@caesarbell caesarbell requested a review from anabellabuckvar May 6, 2024 17:26
@caesarbell caesarbell requested a review from mmeigs May 6, 2024 17:50
Comment on lines 124 to 126
);
if (response.value) {
const job: Job = response.value;
if (response) {
const job: Job = response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change this back!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmeigs even when all the tests are working? I guess you are right, this will keep things consistent.

Comment on lines 108 to 113
const testData = TestDataProvider.getFindOneAndUpdateCallInfo();
const mockVal = { value: getBuildJobPlain() };
const mockVal = getBuildJobPlain();
jest.spyOn(jobRepo, 'notify').mockResolvedValueOnce(true);
dbRepoHelper.collection.findOneAndUpdate.mockResolvedValueOnce(mockVal);

await expect(jobRepo.getOneQueuedJobAndUpdate()).resolves.toEqual(mockVal.value);
await expect(jobRepo.getOneQueuedJobAndUpdate()).resolves.toEqual(mockVal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And change this back, I believe!

@caesarbell caesarbell requested a review from mmeigs May 6, 2024 19:07
Copy link
Contributor

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@seungpark seungpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nitpick below but overall LGTM 👍 thanks for testing and providing changelog documents!

@@ -115,7 +115,7 @@ export class JobRepository extends BaseRepository {

async findOneAndUpdateJob(query): Promise<Job | null> {
const update = { $set: { startTime: new Date(), status: 'inProgress' } };
const options = { sort: { priority: -1, createdTime: 1 }, returnNewDocument: true };
const options = { sort: { priority: -1, createdTime: 1 }, returnNewDocument: true, includeResultMetadata: true };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! think this can be a default option for BaseRepository so we don't have to repeat it on each call

@caesarbell caesarbell merged commit 51032f4 into main May 7, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants