-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(backend-python): add basic telestion library for python #423
base: main
Are you sure you want to change the base?
Conversation
… fix various bugs, add test script for development (will be deleted before merge into main)
…tting up the health check in designated function
…d add basic doc strings
Please also adjust the |
We already set this up when we committed Add a CODEOWNERS file. |
Interesting... For some reason, GitHub seems to believe I'm the owner 🤔 |
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.
Preliminary review – see inline comments.
def build_config(clazz: type[_TelestionConfigT] = None) -> _TelestionConfigT: | ||
if clazz is None: | ||
clazz = TelestionConfig |
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.
Why default to None
instead of TelestionConfig
if you re-set it immediately afterwards?
def _from_env_or_cli(key: str): | ||
return cli_args.get(key, os.environ.get(key, None)) | ||
|
||
config_p = _from_env_or_cli('CONFIG_FILE') |
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.
Why call it config_p
instead of config_file
(or, if you must, config_path
)?
if '://' in url: | ||
_, url = url.split('://', 1) | ||
|
||
return f"nats://{config.username}:{config.password}@{url}" |
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.
Breaks if the username contains a colon (e.g., database:backup
)
return json.loads(msg, **loads_kwargs) | ||
|
||
|
||
def _prepare_nats_url(config: TelestionConfig) -> str: |
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.
Overall, I think we should look at pre-existing URL handling APIs (may that be in the Python core or a library), due to the numerous potential attack vectors currently present in the code.
if not opts.nats or opts.custom_nc is not None: | ||
return service |
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.
Also custom NATS proxies / mocks can connect => this differentiation is unneeded. Quite the opposite: you still need the health check setup to enable testing of that feature.
@@ -0,0 +1,8 @@ | |||
# Telestion Python Backend | |||
|
|||
This repository contains everything to build a working Telestion backend in Python. |
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 repository contains everything to build a working Telestion backend in Python. | |
This repository contains everything to build Telestion backend services in Python. |
Summary
Add Telestion libraries for creating backend libraries in Python.
Details
The provided tools should - like all other backends - provide necessary tooling for parsing and combining various config methods and start the nats client.
Additional information
For testing the backend, a vitual environment with Python 3.12 is recommended (earlier versions might not work). The necessary requirements can be downloaded from the
requirements.txt
.Related links
CLA