-
Notifications
You must be signed in to change notification settings - Fork 8
chore: support static connection info #406
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
|
@rhatgadkar-goog With no PR description I do not yet fully see the benefit or use-case of this change... Is it to allow to customize the certs more easily? |
|
This is a peer of what we did in Go: https://github.com/GoogleCloudPlatform/alloydb-go-connector/blob/main/options.go#L218-L246 The primary use case is for development-time convenience by removing the control plane from the connection flow. I would guess there aren't many production uses for this, but it's a big convenience for development. |
|
|
||
| while True: | ||
| with context.wrap_socket(sock, server_side=True) as ssock: | ||
| with context.wrap_socket(sock, server_side=True) as ssock: |
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.
This change is needed to run multiple tests that use the same proxy_server fixture. Without it, only 1 test can successfully run. The other tests fail with "Connection refused" errors.
tests/unit/conftest.py
Outdated
| thread.start() | ||
| yield thread | ||
| thread.join() | ||
| thread.join(0.1) # wait 100ms to allow the proxy server to start |
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.
This change is required when implementing the change above (swapping the order of while True and with context.wrap_socket) to prevent the test from hanging
ecb6a68 to
a8e5780
Compare
3e74127 to
013375f
Compare
013375f to
6b3290c
Compare
jackwotherspoon
left a comment
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.
I still am not sold on this approach being the best one. There are ways to load static info without changes to the public interface. I would highly suggest looking into them more instead of just doing this purely because the Go Connector did it this way.
One example being I don't think it would be much effort to make a SandBoxClient testing object similar to FakeAlloyDBClient that is wired up to always return static connection info.
alloydb-python-connector/tests/unit/mocks.py
Line 198 in 397d34f
| class FakeAlloyDBClient: |
This way you could write a simple private helper method _create_sandbox_connector that would look something like this:
def _create_sandbox_connector(static_info, *args, **kwargs):
connector = Connector(*args, **kwargs)
client = SandBoxClient(static_info)
connector._client = client
return connectorThis lets you not mess with the public interface for something that will never be used by customers, reducing confusion in the code and making things more readable.
We discussed that for now we will keep the current approach of having the |
jackwotherspoon
left a comment
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 ✅
This is a port of GoogleCloudPlatform/alloydb-go-connector#572 for the AlloyDB Python connector. It allows connecting to a sandbox AlloyDB instance using connection info properties from a JSON file, instead of calling the AlloyDB APIs to get this information (which is done by the internal and lazy caches).
Using a static connection info currently works for the sync client (
pg8000), but doesn't work for the async client (asyncpg). When using the async client, getting timeout errors here, due to ConnectionDoesNotExistError. This is fine for now, as long as we can use the Python connector (using the sync client) to connect to a sandbox AlloyDB instance.