Skip to content

Conversation

addaleax
Copy link
Collaborator

@addaleax addaleax commented Jun 17, 2021

This is fairly functional already, but e.g. the aggregation pipeline builder doesn't appear to work yet, and I have only checked a small subset of Compass's functionality here.

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@addaleax addaleax changed the title [WIP] feat: bump driver to 4.x beta COMPASS-4805 feat: bump driver to 4.x beta COMPASS-4805 Jun 18, 2021
@addaleax addaleax marked this pull request as ready for review June 18, 2021 11:51
@mcasimir
Copy link
Collaborator

the aggregation pipeline builder doesn't appear to work yet

What does it mean? :)

@addaleax
Copy link
Collaborator Author

the aggregation pipeline builder doesn't appear to work yet

What does it mean? :)

@mcasimir It means that the error from the driver could be undefined instead of null for aggregation .toArray(), but that's fixed now (in bbd05d0) :)

"debug": "^4.1.1",
"lodash": "^4.17.15",
"mongodb": "^3.6.3",
"mongodb3": "npm:mongodb@^3.6.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we 'copy-paste' the parse-connection-string.js file and drop this one? We need to rework that to switch it to the new module anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could... but I don't think we'd exactly want that to be the long-term solution here either, it's probably best to do some actual URL parsing based on mongodb-connection-string-url and then actually handle all the transformations here in the connection model directly (assuming that we keep mongodb-connection-model)

Copy link
Collaborator

Choose a reason for hiding this comment

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

but I don't think we'd exactly want that to be the long-term solution here either

Yeah i was thinking the same, we should definitely use the mongodb-connection-string-url module instead.

(err, _client, tunnel, connectionOptions) => {
if (err) {
this._isConnected = false;
this._isConnecting = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :)

@mcasimir
Copy link
Collaborator

the aggregation pipeline builder doesn't appear to work yet

What does it mean? :)

@mcasimir It means that the error from the driver could be undefined instead of null for aggregation .toArray(), but that's fixed now (in bbd05d0) :)

Oh ok :)

Copy link
Collaborator

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

Great! Should we merge now and get this in the next release? Or we prefer to wait?

@addaleax
Copy link
Collaborator Author

Great! Should we merge now and get this in the next release? Or we prefer to wait?

As you prefer :) In any case, we should do at least one beta with this I would say. And I don't know if we want to actually do a compass GA release with a beta of the driver instead of a rc.0?

@mcasimir
Copy link
Collaborator

Great! Should we merge now and get this in the next release? Or we prefer to wait?

As you prefer :) In any case, we should do at least one beta with this I would say. And I don't know if we want to actually do a compass GA release with a beta of the driver instead of a rc.0?

There should be no issue with that. What does @mmarcon says?

@mmarcon
Copy link
Member

mmarcon commented Jun 18, 2021

We are going to GA the shell with a beta of the driver, I think it's fine to ship Compass too.

But yeah, let's definitely do a beta of compass with that before we do a GA.

@addaleax addaleax merged commit 1783af8 into master Jun 18, 2021
@addaleax addaleax deleted the 4805-dev branch June 18, 2021 15:07
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