-
Notifications
You must be signed in to change notification settings - Fork 0
fix/cleanup ci #103
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
fix/cleanup ci #103
Conversation
Neiland85
commented
Dec 11, 2025
- chore: ignore VSCode workspace file
- cleanup: remove duplicated CI workflows and stabilize pipeline
- security: mark public host bindings as intentional (#nosec B104)
- refactor: config model now ignores extra fields + cleanup CI workflows
- ci: temporarily disable Railway deploy (installer broken upstream)
- chore: finalize cleanup before switching to OpenAI dossier work
…-Toolkit into fix/cleanup-ci
| @@ -0,0 +1,22 @@ | |||
| from pydantic import field_validator | |||
Check notice
Code scanning / CodeQL
Unused import Note
| def settings_customise_sources( | ||
| cls, settings_cls, init_settings, env_settings, dotenv_settings, file_secret_settings | ||
| ): |
Check notice
Code scanning / CodeQL
Returning tuples with varying lengths Note
tuple of size 2
tuple of size 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR performs a comprehensive cleanup of the CI/CD pipeline, focusing on stabilizing workflows, improving configuration management, and preparing for Railway deployment. The changes standardize workflow naming conventions (using dashes instead of en-dashes), introduce a multi-stage Docker build for better security and reproducibility, and configure the application settings to ignore extra fields for more robust environment handling.
Key Changes:
- Consolidation and cleanup of GitHub Actions workflows with standardized naming and Railway deployment strategies
- Multi-stage Docker build implementation with Python 3.11, non-root user, and wheel-based dependency installation
- Configuration model refactored to ignore extra fields and handle test environments separately
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
New consolidated quality checks workflow running linters, security scans, and tests across Python 3.11 and 3.12 |
.github/workflows/deploy.yml |
Updated deployment workflow with Railway CLI fallback to API-based deployment, GHCR image push |
.github/workflows/test.yml |
Workflow name standardized from en-dash to hyphen |
.github/workflows/security.yml |
Workflow name standardized from en-dash to hyphen |
deploy_v2.yml |
New Railway deployment workflow using API-only approach (appears to be a duplicate/alternative) |
Dockerfile |
Refactored to multi-stage build with Python 3.11, non-root user, wheel-based installs for security and reproducibility |
.python-version |
Python version pinned to 3.11.8 for consistency |
app/config.py |
Added BaseAppSettings with extra fields ignored, new telemetry-related config fields |
config.py |
Configuration snippet showing pytest-aware settings customization |
app/telemetry.py |
New telemetry module with startup/shutdown event handlers and request metrics logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @app.on_event("startup") | ||
| async def startup_telemetry(): | ||
| logger.info("📊 Telemetry initialized successfully") | ||
| logger.info(f"📍 Application: {app.title} v{app.version}") | ||
|
|
||
| # Add shutdown event for cleanup | ||
| @app.on_event("shutdown") | ||
| async def shutdown_telemetry(): | ||
| logger.info("📊 Telemetry shutdown complete") |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow file uses on_event decorators which are deprecated in FastAPI 0.109.1+. Since the project uses FastAPI 0.116.1 (per requirements.txt), this code should be refactored to use the modern lifespan context manager pattern instead. The current implementation may not work correctly or could be removed in future FastAPI versions.
|
|
||
| env: | ||
| IMAGE_NAME: ghcr.io/${{ github.repository_owner }}/neurobank:${{ github.sha }} | ||
| SERVICE_ID: "REPLACE_ME" # <- luego pones el tuyo |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder SERVICE_ID value "REPLACE_ME" will cause Railway deployments to fail. This should either be replaced with the actual service ID or configured as a GitHub secret and referenced via the secrets context.
| SERVICE_ID: "REPLACE_ME" # <- luego pones el tuyo | |
| SERVICE_ID: ${{ secrets.RAILWAY_SERVICE_ID }} |
| - name: Trigger Railway deployment via API (fallback) | ||
| if: steps.cli_install.outputs.cli == 'false' | ||
| env: | ||
| RAILWAY_TOKEN: ${{ secrets.RAILWAY_TOKEN }} | ||
| run: | | ||
| echo "⚠️ CLI unavailable → Using API fallback mode." | ||
| echo "➜ Deploying image: $IMAGE_NAME" | ||
| curl -X POST "$RAILWAY_API" \ | ||
| -H "Content-Type: application/json" \ | ||
| -H "Authorization: Bearer $RAILWAY_TOKEN" \ | ||
| -d "{ | ||
| \"query\": \"mutation { deployService(input: { serviceId: \\\"$SERVICE_ID\\\", image: \\\"$IMAGE_NAME\\\" }) { id } }\" | ||
| }" |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API fallback deployment logic only triggers when CLI installation fails (cli == 'false'), but not when the CLI deployment itself fails. Since the CLI deployment step has continue-on-error: true, a failed deployment won't trigger the API fallback. Consider changing the condition on line 71 to also check if the CLI deployment succeeded, or always run the API deployment as a backup.
| cors_origins: list[str] = Field(default_factory=list) | ||
|
|
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The field cors_origins is defined twice: once in the base class BaseAppSettings (line 14) and again in the Settings class (line 45 in the full file). This duplication will cause the child class definition to override the parent, potentially leading to confusion. Remove one of the definitions to avoid ambiguity.
| cors_origins: list[str] = Field(default_factory=list) |
| from pydantic import field_validator | ||
|
|
||
| class Settings(BaseSettings): # type: ignore | ||
| model_config = SettingsConfigDict( # type: ignore | ||
| env_file=".env", | ||
| extra="ignore" | ||
| ) |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing required imports for this configuration module. The code references BaseSettings and SettingsConfigDict which are not imported. Add the following imports:
from pydantic_settings import BaseSettings, SettingsConfigDict
| env: | ||
| RAILWAY_TOKEN: ${{ secrets.RAILWAY_TOKEN }} | ||
| run: | | ||
| SERVICE_ID="TU_SERVICE_ID" |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SERVICE_ID is set to a placeholder value "TU_SERVICE_ID" which will cause the Railway deployment to fail. This should either be replaced with the actual service ID or configured as a GitHub secret and referenced via secrets context.
| SERVICE_ID="TU_SERVICE_ID" | |
| SERVICE_ID="${{ secrets.RAILWAY_SERVICE_ID }}" |
| def settings_customise_sources( | ||
| cls, settings_cls, init_settings, env_settings, dotenv_settings, file_secret_settings | ||
| ): |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Settings.settings_customise_sources returns tuple of size 2 and tuple of size 4.
| from pydantic import field_validator | ||
|
|
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'field_validator' is not used.
| from pydantic import field_validator |
|
Merge Summary: Cleanup and Stabilization of CI/CD Pipeline This pull request introduces a complete cleanup and stabilization of the CI/CD workflows across the repository. Key improvements included in this merge: Standardized and cleaned all GitHub Actions workflows Removed deprecated or duplicated CI files Ensured configuration models ignore extra environment fields safely Improved security posture by marking public host bindings as intentional (#nosec B104) Temporarily disabled Railway deployment while the upstream installer remains broken Added formatting consistency (Black + isort) Introduced telemetry initialization and improved internal tooling Finalized all CI-related cleanup before shifting focus to the OpenAI dossier work All checks have passed (linting, quality checks, security scans, test suite), This merge strengthens the repository’s reliability, security, and maintainability moving forward. |