feat(python): add TLS and auth args to getting-started examples#2754
feat(python): add TLS and auth args to getting-started examples#2754saie-ch wants to merge 9 commits intoapache:masterfrom
Conversation
saie-ch
force-pushed
the
feature/add-tls-args-to-example
branch
from
a324506 to
861c7ef
Compare
|
Since we use the examples to validate the behaviour of the sdks, my concern is that currently only the "happy path" of the example working without tls is tested. If we are to merge the changes, I'd appreciate it if the example could be tested with the tls config in CI. |
|
I thought about it, and instead of testing tls using the examples, it would be better to test as an integration test. @saie-ch would you be amenable to writing an integration test that will test tls? |
|
@slbotbm Sure, I can write an integration test for TLS. |
| ] | ||
|
|
||
| [tool.mypy] | ||
| ignore_missing_imports = true |
There was a problem hiding this comment.
what's the reason for this change?
There was a problem hiding this comment.
During the lint job, testcontainers is not available during the mypy check because Docker(uv sync --frozen --extra dev --extra testing) is not installed unless the task is test. This caused mypy to fail with import-not-found.
we can add --extra testing-docker to the lint install step in .github/actions/python-maturin/pre-merge/action.yml and check it once — happy to go with whichever approach you prefer.
There was a problem hiding this comment.
Yes, please update the installation, so the testing-docker is added for both test and lint tasks. If we enable ignoring missing imports, we might miss something in the future.
| await client.connect() | ||
| logger.info("Connected. Logging in user...") | ||
| await client.login_user("iggy", "iggy") | ||
| await client.login_user(args.username, args.password) |
There was a problem hiding this comment.
if the credentials are provided in the connection string, then auto_login is enabled, so calling login is no longer needed as a separate step
| # Connect without TLS to a TLS-enabled server | ||
| # IggyClient constructor requires IP address, not hostname | ||
| client = IggyClient(f"127.0.0.1:{port}") | ||
| await client.connect() |
There was a problem hiding this comment.
In this case, doesn't connect already throw an error?
Also, I think the client set up should be as close as possible (we just want to test the difference tls/no tls), so you should use connection string, but without the tls part.
| ] | ||
|
|
||
| [tool.mypy] | ||
| ignore_missing_imports = true |
There was a problem hiding this comment.
Yes, please update the installation, so the testing-docker is added for both test and lint tasks. If we enable ignoring missing imports, we might miss something in the future.
5 participants
The Python getting-started examples connection settings has no way to configure TLS, authentication credentials.
What changed?
The producer and consumer examples only supported a basic --tcp-server-address argument.
Both examples now accept --tls, --tls-ca-file, --username, and --password arguments, and use IggyClient.from_connection_string() to build a connection string with optional TLS parameters.
Local Execution : Passed
Pre-commit hooks ran
Tested locally against iggy server on 127.0.0.1:8090 without TLS:
python examples/python/getting-started/producer.py --tcp-server-address 127.0.0.1:8090
python examples/python/getting-started/consumer.py --tcp-server-address 127.0.0.1:8090
Producer sent 5 batches of 10 messages (50 total), consumer consumed all 50 messages successfully.
Tested on manged server with --tls , --tls-ca-file, username, password.
examples/python/getting-started/producer.py
--tls
--tcp-server-address xxxxxxxx
--username xxxx
--password xxxxxx
--tls-ca-file xxxxx
Python SDK tests: 18/18 passed