feat: add allowed orgins env var#15
Conversation
Kaiohz
left a comment
There was a problem hiding this comment.
Review PR #15: allowed_origins via env var
📊 Score: 6/10
✅ Points positifs
-
Bonne intention - Rendre les CORS origins configurables est une excellente pratique. Hardcoder
localhost:8030limitait la flexibilité. -
Intégration Pydantic Settings - L'utilisation de
BaseSettingspermet naturellement l'override via variable d'environnementALLOWED_ORIGINS. -
Code propre - L'implémentation est simple et suit le pattern existant du projet.
⚠️ Points à améliorer
1. Changement de port non documenté (8030 → 8080)
# Avant (main.py)
allow_origins=["http://localhost:8030"]
# Après (config.py)
allowed_origins: list[str] = ["http://localhost:8080"]Pourquoi le changement de port ? Si c'est intentionnel (nouveau frontend sur 8080), documenter dans la description de la PR. Sinon, garder 8030 pour la rétrocompatibilité.
2. Pas de support CSV explicite pour ALLOWED_ORIGINS
Avec Pydantic Settings, pour setter plusieurs origins via env var:
# Actuellement (nécessite format JSON)
ALLOWED_ORIGINS='["http://localhost:8080", "https://example.com"]'
# Plus convivial avec un parser
ALLOWED_ORIGINS="http://localhost:8080,https://example.com"Suggestion dans config.py:
from typing import Annotated
from pydantic import field_validator
class Settings(BaseSettings):
allowed_origins: list[str] = ["http://localhost:8080"]
@field_validator("allowed_origins", mode="before")
@classmethod
def parse_origins(cls, v):
if isinstance(v, str):
return [origin.strip() for origin in v.split(",")]
return v3. Typo dans le titre
"orgins" → "origins"
4. Pas de tests
Il manque un test pour valider que le CORS fonctionne avec différentes configurations d'origins.
💡 Suggestions
-
Corriger le port - Soit documenter le changement, soit revenir à
8030(ou le port actuel du frontend) -
Ajouter le validator CSV - Pour faciliter la configuration en production
-
Mise à jour de la doc - Ajouter
ALLOWED_ORIGINSdans un README ou documentation de déploiement -
Tests unitaires:
def test_cors_origins_from_env():
with patch.dict(os.environ, {"ALLOWED_ORIGINS": "http://localhost:3000,https://app.example.com"}):
settings = Settings()
assert settings.allowed_origins == ["http://localhost:3000", "https://app.example.com"]Verdict
PR fonctionnelle mais nécessite des ajustements avant merge. Le changement de port sans explication et l'absence de tests sont les principaux freins.
Actions requises:
- Clarifier le changement de port 8030 → 8080
- Corriger la typo dans le titre
- Ajouter tests CORS
- (Optionnel) Ajouter validator CSV pour plus de flexibilité
No description provided.