elixir-cloud-aai / TESK

GA4GH Task Execution Service Root Project + Deployment scripts on Kubernetes
https://tesk.readthedocs.io
Apache License 2.0
39 stars 29 forks source link

refactor: Tesk app #197

Closed JaeAeich closed 3 months ago

JaeAeich commented 3 months ago

Summary

All the configs and commonly used attributes that are parsed at runtime can be reused with this abstraction, eg the config value.

Summary by Sourcery

This pull request refactors the Tesk application by introducing a new TeskApp class that extends the Foca framework for initializing and running the application. It also adds a custom ConfigNotFoundError for better error handling when configuration files are missing.

sourcery-ai[bot] commented 3 months ago

Reviewer's Guide by Sourcery

This pull request refactors the Tesk application by introducing a new TeskApp class that extends the Foca framework. The changes consolidate the configuration loading and server initialization logic into this new class, simplifying the main application entry point. Additionally, a new custom exception ConfigNotFoundError is added to handle missing configuration files, and a minor formatting issue is fixed in the TesServiceInfo class.

File-Level Changes

Files Changes
tesk/app.py
tesk/tesk_app.py
Refactored the application entry point to use a new TeskApp class, consolidating configuration loading and server initialization logic.

Tips - Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.
JaeAeich commented 3 months ago

Would you care to explain (ideally in the PR description) why you think this change is necessary?

SInce the app will be handeling reading and storing config, I think it would be better if that is done at the instatntiation only and not when the endpoint it called, specifically service-info. While writing that endpoint, I could would;ve had to use ConfigParser from FOCA to read the custom attr in the config, but that felt redundant as that info has aready been parsed in the instantiation of the app then why to rewrite the same logic again.

uniqueg commented 3 months ago

Summary

All the configs and commonly used attributes that are parsed at runtime can be reused with this abstraction, eg the config value.

Summary by Sourcery

This pull request refactors the Tesk application by introducing a new TeskApp class that extends the Foca framework for initializing and running the application. It also adds a custom ConfigNotFoundError for better error handling when configuration files are missing.

  • Enhancements:

    • Refactored the application entry point to use a new TeskApp class for initialization and running.
    • Introduced a custom ConfigNotFoundError for handling missing configuration files.

I suppose the use case is valid - given that config parsing is tied to the FOCA app creation in the current FOCA version, it makes the config not easily accessible outside of app context. However, I think the proper way of handling this is to decouple the config parsing from the app creation in FOCA and provide an easy entry point to the config in FOCA itself (basically what you did but generalized in FOCA). Apps using FOCA would then use two steps to create the app: 1. Parse the config; 2. Create the app based on the parsed config.

That will address your use case without having to put this code in each repo and without having to parse the config twice.

So I propose to close this and consider reusing (parts of) the code in FOCA after the Connexion 3 upgrade.

JaeAeich commented 3 months ago

So I propose to close this and consider reusing (parts of) the code in FOCA after the Connexion 3 upgrade.

Please check changes in #198