elixir-cloud-aai / TESK

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

feat: add FOCA boilerplate #185

Closed JaeAeich closed 1 month ago

JaeAeich commented 1 month ago

Fixes #156

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.21%. Comparing base (f3f2fbc) to head (727bcbb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #185 +/- ## ======================================= Coverage 98.21% 98.21% ======================================= Files 8 8 Lines 561 561 ======================================= Hits 551 551 Misses 10 10 ``` | [Flag](https://app.codecov.io/gh/elixir-cloud-aai/TESK/pull/185/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-cloud-aai) | Coverage Δ | | |---|---|---| | [test_unit](https://app.codecov.io/gh/elixir-cloud-aai/TESK/pull/185/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-cloud-aai) | `98.21% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-cloud-aai#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JaeAeich commented 1 month ago

@uniqueg have you encountered this error.

❯ python3 tesk/app.py
Traceback (most recent call last):
  File "/home/jaeaeich/Desktop/TESK/tesk/app.py", line 7, in <module>
    from connexion import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/__init__.py", line 32, in <module>
    from .apps.flask_app import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/apps/flask_app.py", line 151, in <module>
    class FlaskJSONEncoder(json.JSONEncoder):
                           ^^^^^^^^^^^^^^^^
AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

The fix is in the connexion package to import json rather than falsk.json, but obv this is not the solution. The other is to do update the poetry package connexion, this works but since I dont have connexion as direct dep rather an indirect one via foca idk how to make it stick.

Either way just so that I don;t keep breaking stuff I'll add a CI test to probe the API and see it its working or not, but maybe the solution would be to check this think in FOCA itself.

JaeAeich commented 1 month ago

image what does this mean?

[2024-05-30 15:32:36,244: INFO    ] Registered database 'access_control_db' at URI 'mongodb':'27017' with Flask application. [foca.database.register_mongodb]
[2024-05-30 15:32:36,244: INFO    ] Added database collection 'policy_rules'. [foca.database.register_mongodb]
uniqueg commented 1 month ago

@uniqueg have you encountered this error.

❯ python3 tesk/app.py
Traceback (most recent call last):
  File "/home/jaeaeich/Desktop/TESK/tesk/app.py", line 7, in <module>
    from connexion import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/__init__.py", line 32, in <module>
    from .apps.flask_app import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/apps/flask_app.py", line 151, in <module>
    class FlaskJSONEncoder(json.JSONEncoder):
                           ^^^^^^^^^^^^^^^^
AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

The fix is in the connexion package to import json rather than falsk.json, but obv this is not the solution. The other is to do update the poetry package connexion, this works but since I dont have connexion as direct dep rather an indirect one via foca idk how to make it stick.

Either way just so that I don;t keep breaking stuff I'll add a CI test to probe the API and see it its working or not, but maybe the solution would be to check this think in FOCA itself.

@uniqueg have you encountered this error.

❯ python3 tesk/app.py
Traceback (most recent call last):
  File "/home/jaeaeich/Desktop/TESK/tesk/app.py", line 7, in <module>
    from connexion import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/__init__.py", line 32, in <module>
    from .apps.flask_app import FlaskApp
  File "/home/jaeaeich/Desktop/TESK/.venv/lib/python3.11/site-packages/connexion/apps/flask_app.py", line 151, in <module>
    class FlaskJSONEncoder(json.JSONEncoder):
                           ^^^^^^^^^^^^^^^^
AttributeError: module 'flask.json' has no attribute 'JSONEncoder'

The fix is in the connexion package to import json rather than falsk.json, but obv this is not the solution. The other is to do update the poetry package connexion, this works but since I dont have connexion as direct dep rather an indirect one via foca idk how to make it stick.

Either way just so that I don;t keep breaking stuff I'll add a CI test to probe the API and see it its working or not, but maybe the solution would be to check this think in FOCA itself.

No, I haven't seen the error.

You can't use Connexion 3, this will mess with everything. As you know, I'm working on that, but it will still take a while, maybe some weeks. We could update to the latest Connexion 2 in FOCA, which is 2.14.x - if that helps you.

But if it's just about the typing, try App instead of FlaskApp, or just ignore it if you can. With the upcoming upgrade to Connexion 3, this should be fixed.

JaeAeich commented 1 month ago

I have fixed it with update, so the version in the current 'poetry.lock' work but just so that they don't break with any accidental {up/down}grades I have added a CI check as well, would have added it later but I think it needed rn with this.

uniqueg commented 1 month ago

image what does this mean?

[2024-05-30 15:32:36,244: INFO    ] Registered database 'access_control_db' at URI 'mongodb':'27017' with Flask application. [foca.database.register_mongodb]
[2024-05-30 15:32:36,244: INFO    ] Added database collection 'policy_rules'. [foca.database.register_mongodb]

This is the access control configuration that is automatically set up - this was a huge PR that was going on for a year, and we eventually merged it even though it was still very messy. I will refactor it along the way, such that it doesn't show up if it's not explicitly activated (plus many other necessary improvements). It doesn't make sense to just do that stuff when nobody is asking for it, especially when you didn't even set up a database for handling the permissions.

However, given that it's lazily creating the database connection, as long as you don't try to do any actual DB operations, this shouldn't interfere with anything. So I guess you can leave it for now - unless you actually want to set access control, maybe together with Rahul. You need to do it at some point anyway, and IF it's set up, you could implement 403 in your controllers without revisiting them again later, so it does make some sense to figure this out first.

What speaks against implementing this now, however, is that - as I said - the implementation is still very messy and will very likely undergo considerable changes when I refactor it. It shouldn't change the access policies etc. though, mostly just the configuration and internal implementation in FOCA, so I don't expect that changes on your end would be considerable once its cleaned up in a future FOCA release.

Up to you...

uniqueg commented 1 month ago

I have fixed it with update, so the version in the current 'poetry.lock' work but just so that they don't break with any accidental {up/down}grades I have added a CI check as well, would have added it later but I think it needed rn with this.

Suits me fine. No need to create a release just for that then, before the Connexion 3 migration.

uniqueg commented 1 month ago

Add foca configs.

fixes - #156

I think you could improve on that description a little :upside_down_face:

Something like:

uniqueg commented 1 month ago

Awesome!

I changed the PR title to feat because I think it's worth it :)

Merging now