fix: resolver todos los checks de CI #114
Conversation
- Actualizar tests de logging para nueva firma setup_logging() -> None - Corregir URL del endpoint invoice en test_operator.py (/api/invoice) - Corregir status code esperado 200 -> 201 para POST invoice - Mantener compatibilidad con CodeQL (zero-arg setup_logging) - 34/34 tests passing
There was a problem hiding this comment.
Pull request overview
This PR resolves CI check failures and implements comprehensive improvements to the NeuroBank FastAPI banking application, including Railway deployment optimization, refactored API endpoints, and enhanced configuration management.
Key changes:
- Updated
setup_logging()function signature to returnNoneand fixed related test assertions - Refactored invoice endpoint from
/api/invoice/{invoice_id}to/api/invoicewith proper RESTful design and 201 status code - Migrated to Pydantic v2 settings with enhanced security (repr=False for sensitive fields) and absolute imports
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Minor whitespace formatting adjustment |
| pyproject.toml | Enhanced isort configuration with additional sorting options |
| app/utils/logging.py | Refactored setup_logging with None return type, improved handler management, and better documentation |
| app/tests/test_logging.py | Updated tests to work with new setup_logging signature that doesn't return a logger |
| app/tests/test_telemetry.py | Fixed import ordering per isort configuration |
| app/tests/test_operator.py | Updated tests for new invoice endpoint path and correct 201 status code |
| app/tests/test_main.py | Fixed import ordering per isort configuration |
| app/routers/operator.py | Refactored to use absolute imports, simplified models, moved router prefix to router definition, and redesigned invoice endpoint |
| app/main.py | Improved code organization with section markers, updated logging calls, and simplified router inclusion |
| app/config.py | Migrated to Pydantic v2 SettingsConfigDict, added security with repr=False, and new security_enabled property |
| app/backoffice/router_clean.py | Fixed import ordering per isort configuration |
| app/backoffice/router.py | Enhanced with proper typing, route metadata, and improved HTML structure |
| app/auth/dependencies.py | Minor whitespace formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Prevent duplicate handlers if setup_logging is called twice | ||
| if not root_logger.handlers: |
There was a problem hiding this comment.
The handler guard only prevents adding a new handler when no handlers exist, but it doesn't prevent adding multiple handlers if setup_logging is called multiple times after one handler already exists. Consider using if not any(isinstance(h, logging.StreamHandler) for h in root_logger.handlers) or clearing handlers before adding a new one to ensure idempotency.
| # Prevent duplicate handlers if setup_logging is called twice | |
| if not root_logger.handlers: | |
| # Prevent duplicate StreamHandlers if setup_logging is called multiple times | |
| if not any( | |
| isinstance(h, logging.StreamHandler) and getattr(h, 'stream', None) == sys.stdout | |
| for h in root_logger.handlers | |
| ): |
| @router.post( | ||
| "/invoice/{invoice_id}", | ||
| "/invoice", |
There was a problem hiding this comment.
The endpoint path changed from /invoice/{invoice_id} to /invoice, removing the invoice_id path parameter. While this is a more RESTful design (POST should create resources, not require their ID upfront), it's a breaking API change. Ensure API version management or backward compatibility is in place if there are existing consumers of this endpoint.
|
|
||
| import pytest | ||
| from fastapi import FastAPI | ||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
🚀 Pull Request: Complete Railway Deployment Optimization
📋 Descripción del Cambio
Este PR implementa la solución completa para el problema de crashes de Railway después de 2 minutos, junto con la funcionalidad completa del dashboard administrativo para el sistema bancario NeuroBank FastAPI.
🎯 Problema Solucionado
✅ Solución Implementada
🔧 Cambios Técnicos Implementados
🚂 Railway Deployment
railway.json] Configuración con health checks y restart policiesstart.sh] Script de inicio inteligente con validacionesDockerfile] Optimización single worker + uvloop📊 Admin Dashboard
admin_transactions.html] Panel transacciones completo con Chart.jsadmin_users.html] Gestión usuarios con búsqueda en tiempo realadmin_reports.html] Reportes avanzados con exportación CSV/Excelrouter.py] Conexiones específicas (no más templates genéricos)🔄 CI/CD Pipeline
.github/workflows/production-pipeline.yml] Pipeline de 8 etapas📚 Documentation Suite
HOTFIX_RAILWAY_CRASH.md] Análisis técnico del problema RailwayWORKFLOW.md] Procedimientos de desarrolloGIT_COMMANDS_HOTFIX.md] Comandos de despliegue🧪 Testing & Validation
✅ Funcionalidad Validada
/healthoperativo🔒 Security Checks
⚡ Performance Tests
🎯 Business Impact
🚀 Deployment Instructions
Pre-merge Checklist
RAILWAY_TOKENconfigurado en GitHub SecretsPost-merge Actions
main👥 Review Requirements
🔍 Areas de Focus para Review
railway.jsonystart.sh🎯 Expected Reviewers
📝 Additional Notes
🔄 Future Improvements
📚 Related Documentation
✅ Ready to Merge Criteria
🎉 Este PR convierte NeuroBank FastAPI en una aplicación bancaria de nivel empresarial con despliegue automático y funcionalidad completa!