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

feat(backend-python): add basic telestion library for python #423

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cb0s
Copy link
Member

@cb0s cb0s commented Mar 13, 2024

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

  • I have signed the individual contributor's license agreement and sent it to the board of the WüSpace e. V. organization.

@pklaschka
Copy link
Member

Please also adjust the .github/CODEOWNERS before we merge this – I would rather not own the Python library, considering that the last time I did major work in Python was over two years ago 🙈.

@cb0s
Copy link
Member Author

cb0s commented Mar 16, 2024

Please also adjust the .github/CODEOWNERS before we merge this – I would rather not own the Python library, considering that the last time I did major work in Python was over two years ago 🙈.

We already set this up when we committed Add a CODEOWNERS file.

@pklaschka
Copy link
Member

Please also adjust the .github/CODEOWNERS before we merge this – I would rather not own the Python library, considering that the last time I did major work in Python was over two years ago 🙈.

We already set this up when we committed Add a CODEOWNERS file.

image

Interesting... For some reason, GitHub seems to believe I'm the owner 🤔

@pklaschka
Copy link
Member

image
Ah, wait... It's only because it includes documentation (the README.md) – disregard what I said 🙈

Copy link
Member

@pklaschka pklaschka left a 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.

Comment on lines +32 to +34
def build_config(clazz: type[_TelestionConfigT] = None) -> _TelestionConfigT:
if clazz is None:
clazz = TelestionConfig
Copy link
Member

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')
Copy link
Member

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}"
Copy link
Member

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:
Copy link
Member

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.

Comment on lines +104 to +105
if not opts.nats or opts.custom_nc is not None:
return service
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This repository contains everything to build a working Telestion backend in Python.
This repository contains everything to build Telestion backend services in Python.

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