-
Notifications
You must be signed in to change notification settings - Fork 20
Add mongodb driver checks #2363
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
base: development/8.1
Are you sure you want to change the base?
Conversation
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
1e3e78b
to
d0ba24d
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
d0ba24d
to
aa8e269
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development/8.1 #2363 +/- ##
===================================================
+ Coverage 68.69% 69.33% +0.63%
===================================================
Files 218 218
Lines 17533 17573 +40
Branches 3579 3612 +33
===================================================
+ Hits 12045 12184 +139
+ Misses 5472 5373 -99
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0954ce7
to
1c8236e
Compare
/create_integration_branches |
1c8236e
to
2de80d5
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
4919cf3
to
a0dcf44
Compare
History mismatchMerge commit #2de80d5a6cbb18f8df31a82d3029d2462da8b1e3 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
a0dcf44
to
507987f
Compare
507987f
to
80fcfbd
Compare
.then(result => { | ||
if (result.matchedCount === 0 && result.modifiedCount === 0 && result.upsertedCount === 0) { | ||
log.debug('putBucketAttributes: failed to update bucket', { bucketName }); | ||
return cb(errors.NoSuchBucket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
- this is really a case which "should not" (but does) happen: may be best to keep it as InternalError, possibly customized with an indication of the "mongo" issue (
errors.InternalError.customize("mongo failed to match, update and upsert")
) - maybe we can dedup this case of error detection after an update operation, by introducing a
processUpdateResult(result, cb)
function?
Catching errors might not be enough: in some cases, a call can succeed yet the result is not what we expect: e.g. no document matching the filters. This leads to inconsistent behaviors where the API thinks the operation succeed while it didn't...
In this PR we identify and fix multiple places where we are ignoring MongoDB return flags. We add the error handling and multiple tests are added, as today some methods are not unit tested at all.
Issue: ARSN-488