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

Poetry project #2

Merged
merged 18 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: CI

on:
push:

jobs:
check:
runs-on: ubuntu-24.04
timeout-minutes: 10

steps:
- uses: actions/checkout@v1

- name: build
run: docker compose build check

- name: check
run: docker compose run --rm check

- name: test
run: docker compose run --rm check poetry run pytest tests/
29 changes: 29 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,33 @@ geOrchestra Maelstro is an application which helps synchronise geonetwork and ge
Refer to documentation from https://github.com/georchestra/docker/tree/master?tab=readme-ov-file#on-linux to trust caddy certificate

Also you need to run few commands before to start documented here : [georchestra/README.md](georchestra/README.md)

## Backend

The folder [backend](backend) contains the API written with FastAPI.

### Access

In the global dev composition, the backend is accessible via the https gateway:
https://georchestra-127-0-0-1.nip.io/maelstro-backend/

### SwaggerUI

FastAPI automatically builds a swagger API web interface which can be found at
https://georchestra-127-0-0-1.nip.io/maelstrob/docs

Here the various API entrypoints can be tested

## CI

Automatic code quality checks are implemented in the CI.

The code test can be launched manually via the docker command below.
```
docker compose run --rm check
```

In case formatting issues are found, the code can be auto-fixed with:
```
docker compose run --rm check /app/maelstro/scripts/code_fix.sh
```
1 change: 1 addition & 0 deletions backend/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Dockerfile
26 changes: 23 additions & 3 deletions backend/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,8 +1,28 @@
FROM python:3.12-slim-bullseye
FROM python:3.12-slim-bullseye AS poetry

RUN pip install "fastapi[standard]" requests
RUN --mount=type=cache,target=/root/.cache \
pip install poetry
RUN poetry config virtualenvs.create false

RUN mkdir /app
WORKDIR /app

COPY pyproject.toml /app

FROM poetry as server
RUN --mount=type=cache,target=/root/.cache \
poetry install --no-root

COPY . /app

WORKDIR /app
RUN --mount=type=cache,target=/root/.cache \
poetry install

CMD ["serve_prod"]

FROM server as check
Copy link
Member

@edevosc2c edevosc2c Jan 22, 2025

Choose a reason for hiding this comment

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

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?


RUN --mount=type=cache,target=/root/.cache \
poetry install --no-root --with check

CMD ["/app/maelstro/scripts/code_check.sh"]
1 change: 1 addition & 0 deletions backend/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Maelstro sync app for Georchestra
5 changes: 0 additions & 5 deletions backend/health_check.py

This file was deleted.

62 changes: 62 additions & 0 deletions backend/maelstro/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
"""
Entry point scripts for maelstro backend server
"""

import uvicorn
from fastapi_cli.utils.cli import get_uvicorn_log_config


def dev():
Copy link
Contributor

@arnaud-morvan arnaud-morvan Jan 21, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's comfortable for now, we can discuss this later

"""
Dev server entrypoint:
special configuration:
- listens only on localhost
- restarts on code change
"""
uvicorn.run(
app="maelstro.main:app",
host="127.0.0.1",
port=8000,
reload=True,
workers=None,
root_path="",
# proxy_headers=proxy_headers,
log_config=get_uvicorn_log_config(),
)


def docker_dev():
"""
Dev server entrypoint for running the server inside a docker container:
special configuration:
- restarts on code change
"""
uvicorn.run(
app="maelstro.main:app",
host="0.0.0.0",
port=8000,
reload=True,
workers=None,
root_path="",
# proxy_headers=proxy_headers,
log_config=get_uvicorn_log_config(),
)


def prod():
"""
Server entrypoint for running the server inside a docker container:
"""
uvicorn.run(
app="maelstro.main:app",
host="0.0.0.0",
port=8000,
workers=None, # to be configured
root_path="",
# proxy_headers=proxy_headers,
log_config=get_uvicorn_log_config(),
)


if __name__ == "__main__":
docker_dev()
93 changes: 93 additions & 0 deletions backend/maelstro/main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
"""
Main backend app setup
"""

from typing import Annotated
from fastapi import FastAPI, Request, Response, Header


app = FastAPI(root_path="/maelstro-backend")

app.state.health_countdown = 5


@app.head("/")
@app.get("/")
def root_page():
"""
Hello world dummy route
"""
return {"Hello": "World"}


@app.get("/user")
def user_page(
sec_username: Annotated[str | None, Header(include_in_schema=False)] = None,
sec_org: Annotated[str | None, Header(include_in_schema=False)] = None,
sec_roles: Annotated[str | None, Header(include_in_schema=False)] = None,
sec_external_authentication: Annotated[
str | None, Header(include_in_schema=False)
] = None,
sec_proxy: Annotated[str | None, Header(include_in_schema=False)] = None,
sec_orgname: Annotated[str | None, Header(include_in_schema=False)] = None,
):
"""
Display user information provided by gateway
"""
return {
"username": sec_username,
"org": sec_org,
"roles": sec_roles,
"external-authentication": sec_external_authentication,
"proxy": sec_proxy,
"orgname": sec_orgname,
}


# pylint: disable=fixme
# TODO: deactivate for prod
@app.head("/debug")
@app.get("/debug")
@app.put("/debug")
@app.post("/debug")
@app.delete("/debug")
@app.options("/debug")
@app.patch("/debug")
def debug_page(request: Request):
arnaud-morvan marked this conversation as resolved.
Show resolved Hide resolved
"""
Display details of query including headers.
This may be useful in development to check all the headers provided by the gateway.
This entrypoint should be deactivated in prod.
"""
return {
**{
k: request[k]
for k in [
"type",
"asgi",
"http_version",
"server",
"client",
"scheme",
"root_path",
]
},
"method": request.method,
"url": request.url,
"headers": dict(request.headers),
"query_params": request.query_params.multi_items(),
}


@app.get("/health")
def health_check(response: Response):
"""
Health check to make sure the server is up and running
For test purposes, the server is reported healthy only from the 5th request onwards
"""
status: str = "healthy"
if app.state.health_countdown > 0:
app.state.health_countdown -= 1
response.status_code = 404
status = "unhealthy"
return {"status": status, "user": None}
10 changes: 10 additions & 0 deletions backend/maelstro/scripts/code_check.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/bin/bash

cd "$(dirname "$0")"/..

poetry run black --check . && \
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider using pre-commit: https://pre-commit.com/

IMHO those files don't need to be inside the Python package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

poetry run mypy . && \
poetry run pyflakes . && \
poetry run pylint .

! (( $? & 7 )) # mask exit code for minor findings (refactor, convention, usage)
5 changes: 5 additions & 0 deletions backend/maelstro/scripts/code_fix.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/bash

cd "$(dirname "$0")"/..

poetry run black .
7 changes: 7 additions & 0 deletions backend/maelstro/scripts/health_check.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/usr/local/bin/python

import requests


def check():
assert requests.get("http://localhost:8000/health").status_code == 200
19 changes: 0 additions & 19 deletions backend/main.py

This file was deleted.

37 changes: 37 additions & 0 deletions backend/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
[project]
name = "maelstro"
version = "0.1.0"
description = ""
authors = [
{name = "Your Name",email = "[email protected]"}
]
readme = "README.md"
requires-python = ">=3.12"
dependencies = [
"fastapi[standard] (>=0.115.6,<0.116.0)",
"requests (>=2.32.3,<3.0.0)",
]

[tool.poetry.group.check]
optional = true

[tool.poetry.group.check.dependencies]
mypy=">=1.14.1,<2.0.0"
types-requests=">=2.32.0.20241016,<3.0.0.0"
pylint=">=3.3.3,<4.0.0"
pyflakes=">=3.2.0,<4.0.0"
black=">=24.10.0,<25.0.0"
pytest = "^8.3.4"

[tool.poetry.scripts]
health_check = "maelstro.scripts.health_check:check"
serve_dev = "maelstro:dev"
serve_docker_dev = "maelstro:docker_dev"
serve_prod = "maelstro:prod"

[build-system]
requires = ["poetry-core>=2.0.0,<3.0.0"]
build-backend = "poetry.core.masonry.api"

[tool.pylint.format]
max-line-length = "88"
Empty file added backend/tests/__init__.py
Empty file.
12 changes: 12 additions & 0 deletions backend/tests/test_API.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from fastapi.testclient import TestClient

from maelstro.main import app


client = TestClient(app)


def test_read_main():
response = client.get("/")
assert response.status_code == 200
assert response.json() == {'Hello': 'World'}
24 changes: 14 additions & 10 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,26 @@ include:

services:
maelstro-back:
build: ./backend
build:
context: ./backend
target: server
volumes:
- ./backend:/app
healthcheck:
Copy link
Member

Choose a reason for hiding this comment

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

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.

https://docs.docker.com/reference/dockerfile/#healthcheck

test: "/app/health_check.py"
test: "poetry run health_check"
interval: 10s
retries: 5
start_period: 10s
timeout: 10s

ports:
- 8000:8000

command:
- fastapi
- dev
- --host
- 0.0.0.0
- main.py
- serve_docker_dev

check:
profiles:
- check
build:
context: ./backend
target: check
volumes:
- ./backend:/app
1 change: 0 additions & 1 deletion georchestra/geor-compose.override.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,3 @@ services:
retries: 10
env_file:
- .envs-common

2 changes: 0 additions & 2 deletions georchestra/geor-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -251,5 +251,3 @@ services:
discovery.type: single-node
ES_JAVA_OPTS: -Xms512m -Xmx512m
restart: no


Loading