Poetry project #2
Conversation
mki-c2c
force-pushed
the
poetry_project
branch
from
cfd4502 to
e63a7e8
Compare
mki-c2c
force-pushed
the
poetry_project
branch
from
5b29401 to
e7fb879
Compare
|
|
||
| cd "$(dirname "$0")"/.. | ||
|
|
||
| poetry run black --check . && \ |
There was a problem hiding this comment.
We could consider using pre-commit: https://pre-commit.com/
IMHO those files don't need to be inside the Python package.
There was a problem hiding this comment.
in this case, pre-commit would be too cumbersome
And for the scripts I really wonder. They are placed inside the package since they use the package dependencies. This is a hard choice.
There was a problem hiding this comment.
I understand you need it in Docker images, but having them in Python package seems weird to me. BTW not sure it is really important.
| from fastapi_cli.utils.cli import get_uvicorn_log_config | ||
|
|
||
|
|
||
| def dev(): |
There was a problem hiding this comment.
Not sure we really need those different entrypoints.
I would consider using environment variables or overwriting command in docker-compose.override.yaml
Note that such env vars might already exists in uvicorn
There was a problem hiding this comment.
it's comfortable for now, we can discuss this later
mki-c2c
force-pushed
the
poetry_project
branch
from
c70e9e0 to
88e024b
Compare
backend/maelstro/main.py
Outdated
| from fastapi import FastAPI, Request, Response, Header | ||
|
|
||
|
|
||
| app = FastAPI(root_path="/maelstrob") |
There was a problem hiding this comment.
can we consider renaming the path to maelstro-backend ?
backend/maelstro/main.py
Outdated
| """ | ||
| Check if the user is authorized for the maestro sync platform via the MAELSTRO role | ||
| """ | ||
| return {"is_authorized": "ROLE_MAELSTRO" in sec_roles.split(";")} |
There was a problem hiding this comment.
IMO, this is not the application's role to check this, rather the gateway or SP's
cc @f-necas
There was a problem hiding this comment.
justement sec_roles est produit par la gateway, qui a vérifié que l'utilisateur est bien authentifié. Je suis confus alors.
bien sur, il faudrait mettre la constante "ROLE_MAELSTRO" dans un fichier de conf
There was a problem hiding this comment.
Tu peux définir directment dans la gateway ou SP si le rôle est correct
https://github.com/georchestra/georchestra/blob/master/security-proxy/README.md?plain=1#L29
https://github.com/georchestra/georchestra-gateway/blob/main/docs/access-rules.adoc?plain=1#L68
There was a problem hiding this comment.
Oui, si j'ai bien compris les requêtes n'arrivent ici que si l'utilisateur à le role, sinon c'est le SP ou la gateway qui renvoie une erreur.
Je ne comprends pas bien non plus l'utilité de cet entrypoint.
mki-c2c
force-pushed
the
poetry_project
branch
from
d53cfd6 to
c53abc2
Compare
| build: ./backend | ||
| build: | ||
| context: ./backend | ||
| target: server | ||
| volumes: | ||
| - ./backend:/app | ||
| healthcheck: |
There was a problem hiding this comment.
Usually you include the healthcheck into the Dockerfile not the healthcheck section of the docker composition.
Adding health check to the Dockerfile, will make the health-check part of the image, so that anyone pulling the image from the registry will get the health check by default.
|
|
||
| CMD ["serve_prod"] | ||
|
|
||
| FROM server as check |
There was a problem hiding this comment.
In my opinion, having multiple docker images from the same Dockerfile using targets makes it very confusing. And you can't just do a simple "docker build -t XX ."
Why not having two Dockerfile but one major one that build the maelstro server docker image. And then another Dockerfile for "check" that reference the docker image built for maelstro server?
setup quality tools and dependencies with poetry