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

Feature request: allow extra options for storage backends #40

Merged
merged 4 commits into from
Sep 3, 2021
Merged

Feature request: allow extra options for storage backends #40

merged 4 commits into from
Sep 3, 2021

Conversation

maxxiefjv
Copy link
Contributor

@maxxiefjv maxxiefjv commented Sep 1, 2021

I would like to allow some way of supporting a TLS connection, specifying the certificates. This is a draft PR meant for discussion

Edit:

As it is about a RedisWrapper this solution is probably better:

    def __init__(self, db_uri, collection, ttl=None, options={}):
        if not _has_redis:
            raise ImportError("redis module is required but it is not available")
        self._db = Redis.from_url(db_uri, decode_responses=True)
        self._db = Redis.from_url(db_uri, decode_responses=True, **options)
        ...

@maxxiefjv maxxiefjv changed the title allow from_uri SSL param options Feature request: allow from_uri SSL param options Sep 1, 2021
@c00kiemon5ter
Copy link
Member

I think this looks ok

@maxxiefjv maxxiefjv marked this pull request as ready for review September 3, 2021 11:00
@maxxiefjv
Copy link
Contributor Author

@c00kiemon5ter I updated the code to your request. I agree that its best to handle this situation as most defaults appear to be None

@c00kiemon5ter c00kiemon5ter changed the title Feature request: allow from_uri SSL param options Feature request: allow extra options for storage backends Sep 3, 2021
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@@ -120,10 +136,21 @@ class RedisWrapper(StorageBase):
Supports JSON-serializable data types.
"""

def __init__(self, db_uri, collection, ttl=None):
def __init__(
self, db_uri, *, db_name=None, collection, ttl=None, extra_options=None
Copy link
Member

Choose a reason for hiding this comment

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

Note: set all but the first arg to RedisWrapper to be keyword-only args.

Added db_name as the redis db index.

Copy link
Member

Choose a reason for hiding this comment

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

Now, the MongoWrapper and RedisWrapper initialization interfaces are the same.

(["mongodb://localhost/pyop"], {"collection": "test", "ttl": 3}),
(["mongodb://localhost"], {"db_name": "pyop", "collection": "test"}),
(["mongodb://localhost", "test", "pyop"], {}),
(["mongodb://localhost/pyop", "test"], {}),
(["mongodb://localhost/pyop"], {"db_name": "other", "collection": "test"}),
(["redis://localhost/0"], {"db_name": "pyop", "collection": "test"}),
Copy link
Member

Choose a reason for hiding this comment

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

db_name is now valid for redis.

@c00kiemon5ter c00kiemon5ter merged commit 0db2f2f into IdentityPython:master Sep 3, 2021
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.

2 participants