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

mongo_future_return: MongoDb Connection string support #48371

Merged
merged 5 commits into from Jul 5, 2018
Merged

mongo_future_return: MongoDb Connection string support #48371

merged 5 commits into from Jul 5, 2018

Conversation

jyurdal
Copy link

@jyurdal jyurdal commented Jun 29, 2018

What does this PR do?

PR adds MongoDB connection string support (URI), what enables using replica set and other connection options.
Those features are already supported in pymongo so this PR only adds URI config and passes it to pymongo.

What issues does this PR fix or reference?

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

else:
conn = pymongo.Connection(host, port)
mdb = conn[db_]
if float(version) > 2.3:
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to use salt.utils.versions.LooseVersion instead of float(), to be uniform with other version checks throughout salt's codebase?

that's valid for both mongo_future_return.py and mongo_returner.py

Copy link
Author

Choose a reason for hiding this comment

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

I can change it to the LooseVersion.

I did not want to change code not related to the URI feature but I could imagine installing salt 2018 requires lots of up to date python modules so it could be the time to remove support for pymongo < 2.3
In that case port, host and other configs could be removed and only connection string would be supported ( you can easily put all previously used options into connection string format)
If you and other salt contributors think it is OK, I can make the changes.

Copy link
Contributor

@rares-pop rares-pop Jun 30, 2018

Choose a reason for hiding this comment

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

I'm not that familiar with mongodb and I don't know the users' existing code-base, so I can't recommend anything. Maybe is okay to deprecate it (a sudden removal seems.. sudden).

I thought it's a low-hanging-fruit and seems among the last places where salt has a custom version compare. I think it can easily be done independent of this change though.

I'm not an official reviewer, so it's not in my power to approve things.

LE: I did look in their documentation and they support both host:port and URI when creating a client:
http://api.mongodb.com/python/current/tutorial.html#making-a-connection-with-mongoclient
However, their c++11 driver only works with URI

What if instead of having a warning and having the URI taking precedence over the host:port config of the client, you just error out so the user can fix his configuration not to contain both?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, so all in all:

  • depreciate host and port configuration ( but still support it)
  • error on both uri and host
  • any comments on removing 2.3 support anyone?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, lets use LooseVersion.

We shouldn't remove support for older versions if we can help it. And I would say we shouldn't remove support for older configurations when it isn't absolutely necessary.

@rallytime rallytime requested a review from a team July 2, 2018 17:50
Copy link
Contributor

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

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

Just a couple changes.

Thanks!
Daniel


.. code-block:: yaml

mongo.uri: URI
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide an example of the uri format.

Thanks,
Daniel

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Author

@jyurdal jyurdal Jul 5, 2018

Choose a reason for hiding this comment

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

I have updated doc, and changed LooseVersion in mongo_future_return and mongo_return. I did some more extra handling so that only uri or host can be specified in the config file.
Tested with pymongo 3.6 and 2.3

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect, thanks!

else:
conn = pymongo.Connection(host, port)
mdb = conn[db_]
if float(version) > 2.3:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, lets use LooseVersion.

We shouldn't remove support for older versions if we can help it. And I would say we shouldn't remove support for older configurations when it isn't absolutely necessary.

mdb.authenticate(user, password)
mdb = conn[db_]
if user and password:
mdb.authenticate(user, password)

if indexes:
if float(version) > 2.3:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you wouldn't mind, it would be great to move all these other version comparisons to using LooseVersion as well.

Copy link
Contributor

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Nice!

@rallytime rallytime merged commit 8731f42 into saltstack:develop Jul 5, 2018
@jyurdal jyurdal deleted the mongo_uri_feature branch July 5, 2018 19:42
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.

None yet

4 participants