Skip to content

Conversation

@autarch
Copy link
Collaborator

@autarch autarch commented Aug 22, 2024

This also includes some linting fixes and some tooling updates.

@autarch autarch changed the title Exclude all Mongosync internal DBs, including those used by the embedded verifier REP_4790 Exclude all Mongosync internal DBs, including those used by the embedded verifier Aug 22, 2024
@autarch autarch changed the title REP_4790 Exclude all Mongosync internal DBs, including those used by the embedded verifier REP-4790 Exclude all Mongosync internal DBs, including those used by the embedded verifier Aug 22, 2024
This is mostly for my own benefit, since this repo doesn't have any support for installing precious
and other tools.
@autarch autarch marked this pull request as ready for review August 22, 2024 17:39
@autarch autarch requested a review from mmcclimon August 22, 2024 17:39
@autarch
Copy link
Collaborator Author

autarch commented Aug 22, 2024

I ran the tests locally and they passed. This repo doesn't run PRs in CI :(

Copy link
Collaborator

@mmcclimon mmcclimon left a comment

Choose a reason for hiding this comment

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

I think this is not quite right, and that we should a) add a test to confirm that it's not right (I suspect right now it's excluding the regex but not admin or something), and b) fix it. Thanks!

Comment on lines 36 to 37
{"name", bson.D{{"$nin", excludedDBs}}},
{"name", bson.D{{"$nin", []primitive.Regex{{Pattern: MongosyncMetaDBsPattern}}}}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is surely wrong, isn't it? You can't provide the name key more than once. I think you want a single $nin []any slice with all of the excludedDBs and also the regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seemed to work, though I realized we weren't actually testing all the excludes. But even adding some more tests, it still seemed to work. But I fixed it anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird! I wonder if the driver does something weird here. Oh well, thanks all the same.

@autarch autarch requested a review from mmcclimon August 22, 2024 18:04
Copy link
Collaborator

@mmcclimon mmcclimon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@autarch autarch merged commit e2fb7cd into main Aug 22, 2024
@autarch autarch deleted the REP-4790 branch August 22, 2024 18:22
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