-
Notifications
You must be signed in to change notification settings - Fork 911
GODRIVER-1469 ensure gridfs index checking supports indexes created i… #341
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
Conversation
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.
As a starting point, I'm finding the code for createIndexIfNotExists
hard to read because the variable names get tricky when we do multiple BSON transformations. I think it would help if we added a new function called compareIndexDocs(expected, actual bsoncore.Document)
that did both a bytes.Equal
check as well as a semantic element-wise check. I want to get your opinion on this before continuing the review.
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.
One nit, but otherwise LGTM. Have you verified that the test fails in our existing implementation?
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.
Looks good! Just a couple of tests that I would like to see added.
@iwysiu The "config file was invalid" error from Evergreen can be fixed by rebasing on master and force-pushing. The Evergreen team invalidated the |
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.
LGTM!
…n the shell