test CI event PR to develop - #1
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request represents a major infrastructure and CI/CD pipeline refactoring with the title "test CI event PR to develop". Despite the casual title, it contains significant production-impacting changes including:
Changes:
- Complete CI/CD pipeline overhaul: removed old deployment workflows, added new semantic-release flow with staging and production pipelines
- Test infrastructure reorganization: moved from flat test structure to layered unit/integration tests with proper fixtures and database rollback
- Infrastructure hardening: added KMS encryption, secret rotation, VPC flow logs, security group restrictions, and CloudWatch log retention
- Frontend build changes: switched from http-server to nginx, updated Angular dependencies, added ESLint configuration
- Removed legacy tooling: deleted tools/ scripts, multiple documentation files, and E2E test infrastructure
Reviewed changes
Copilot reviewed 100 out of 103 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/*.yml | Deleted old deployment workflows, added new staging/release/dependabot workflows |
| infra/modules/* | Added security features: KMS policies, secret rotation, VPC flow logs, stricter security groups |
| backend/tests/* | Reorganized to unit/integration layers with proper fixtures and rollback |
| backend/Dockerfile | Added start.sh for migrations, updated poetry install flags |
| frontend/* | Switched to nginx, updated dependencies, added ESLint config |
| tools/* | Deleted validate-secrets.sh, lint-scripts.sh and related documentation |
| docs/* | Deleted TEST_SUITE_SUMMARY.md, MAKE_REFERENCE.md |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource "aws_cloudwatch_log_group" "rds" { | ||
| name = "/aws/rds/${var.project_name}" | ||
| retention_in_days = var.log_retention_days | ||
| retention_in_days = 365 |
There was a problem hiding this comment.
The log retention is hardcoded to 365 days, overriding the configurable var.log_retention_days that was previously used. This removes flexibility for different environments (e.g., shorter retention in dev/staging, longer in production for compliance).
Consider restoring the variable or at minimum documenting why 1 year is a fixed requirement.
| - name: Load staging environment | ||
| run: | | ||
| grep -v '^\s*#' config/.env.staging | grep -v '^\s*$' >> $GITHUB_ENV |
There was a problem hiding this comment.
The workflow uses grep -v '^\s*#' config/.env.staging to load environment variables. However, this approach has several issues:
- It doesn't handle inline comments (e.g.,
VAR=value # comment) - It may expose secrets in GitHub Actions logs if the file contains sensitive data
- Non-secret configuration should be in workflow variables, not .env files
Consider using GitHub Environment variables instead for non-secret configuration, and only reference secrets via ${{ secrets.* }}.
| @@ -0,0 +1,20 @@ | |||
| { | |||
There was a problem hiding this comment.
The PR description says "test CI event PR to develop" which doesn't adequately describe the massive infrastructure and testing refactoring contained in this PR. This makes it very difficult for reviewers to understand the purpose and scope of changes.
A production PR of this magnitude should have:
- Clear description of what problems are being solved
- List of major changes with rationale
- Migration/deployment notes
- Breaking changes clearly identified
- Rollback plan
| resource "aws_secretsmanager_secret_rotation" "db_password" { | ||
| secret_id = aws_secretsmanager_secret.db_password.id | ||
| rotation_rules { | ||
| automatically_after_days = 30 | ||
| } | ||
| } |
There was a problem hiding this comment.
The aws_secretsmanager_secret_rotation resource is configured without a rotation_lambda_arn. AWS Secrets Manager automatic rotation requires a Lambda function to perform the actual rotation. Without this, the rotation will fail at runtime.
You need to either:
- Add a rotation_lambda_arn pointing to a Lambda function that handles the database password rotation
- Remove this resource if rotation is not yet implemented
- Use AWS-managed rotation by specifying rotate_immediately and hosted_rotation_type for supported databases
| resource "aws_secretsmanager_secret_rotation" "jwt_secret" { | ||
| secret_id = aws_secretsmanager_secret.jwt_secret.id | ||
| rotation_rules { | ||
| automatically_after_days = 30 | ||
| } | ||
| } |
There was a problem hiding this comment.
The JWT secret rotation is configured without a rotation_lambda_arn. AWS Secrets Manager automatic rotation requires a Lambda function to perform the rotation. Additionally, rotating JWT secrets requires careful coordination with the application to ensure tokens signed with the old secret are still valid during the rotation window.
Consider:
- Implementing a Lambda function for rotation
- Supporting multiple concurrent JWT secrets in the application
- Or removing this rotation configuration until the rotation strategy is fully implemented
| depends_on = [ | ||
| aws_secretsmanager_secret_version.db_password, | ||
| ] | ||
| } |
There was a problem hiding this comment.
The removed dependency on aws_secretsmanager_secret_version.db_password in the depends_on block could cause timing issues. The RDS instance may attempt to start before the secret is fully created and available, leading to deployment failures.
The depends_on should be restored to ensure the secret version exists before RDS instance creation.
| pip install poetry && \ | ||
| poetry config virtualenvs.create false && \ | ||
| poetry install --no-dev --no-directory | ||
| poetry install --no-interaction --no-ansi --with dev --no-root |
There was a problem hiding this comment.
The poetry install command now uses --with dev flag, which will install development dependencies in the production image. This increases the image size and attack surface unnecessarily.
Change --with dev to --only main or --without dev to exclude development dependencies from production builds.
| poetry install --no-interaction --no-ansi --with dev --no-root | |
| poetry install --no-interaction --no-ansi --only main --no-root |
| resource "aws_s3_bucket_logging" "alb_logs" { | ||
| bucket = aws_s3_bucket.alb_logs.id | ||
|
|
||
| target_bucket = aws_s3_bucket.alb_logs.id | ||
| target_prefix = "access-logs/" | ||
| } |
There was a problem hiding this comment.
The S3 bucket logging configuration creates a circular dependency by having the bucket log to itself with prefix "access-logs/". While technically possible, this is not a best practice and can lead to exponential log growth (logs of logs of logs...).
Best practice is to create a separate centralized logging bucket, or at minimum use AWS CloudTrail for S3 data events instead of bucket-level access logging.
## 1.0.0 (2026-03-24) * Add comprehensive Checkov security fixes documentation ([29bdff9](29bdff9)) * add dependabot for dependency management ([94d76df](94d76df)) * add dev env in workflow ([1654441](1654441)) * add dev env in workflow ([ebf6484](ebf6484)) * add dev env in workflow ([e2725cc](e2725cc)) * add dev env in workflow ([b550293](b550293)) * add fraontend readme ([dbb0f2a](dbb0f2a)) * add manuel trigger in CI ([42f8f11](42f8f11)) * add timeout minute and correcte ci ([cd22110](cd22110)) * add timeout minute and correcte ci ([6711941](6711941)) * add timeout minute and correcte ci ([89353e2](89353e2)) * add two actions ([e901ba5](e901ba5)) * apply change te terraform code for security check ([4e6d4cc](4e6d4cc)) * change ci only to test branch for testing ([64a2a47](64a2a47)) * CI correction ([d69eb58](d69eb58)) * CI correction ([e2f9870](e2f9870)) * create ec2 workflow ([841d427](841d427)) * delete all terraform scripts ([46ef918](46ef918)) * delete config.json ([092f560](092f560)) * delete md file ([df209f3](df209f3)) * delete secrity check for the moment ([e2b2e66](e2b2e66)) * dev corrected ([4b57119](4b57119)) * dev corrected ([40cd9d0](40cd9d0)) * first commit repo setup ([1c2b772](1c2b772)) * first version backend and frontend ([e52ed6f](e52ed6f)) * first version backend with test and db model ([fab5117](fab5117)) * fix aws AUTH ([7753a8d](7753a8d)) * fix aws AUTH ([e09a85e](e09a85e)) * fix AWS auth ([0486a02](0486a02)) * fix backend CI ([f10979e](f10979e)) * fix backend CI ([e28ea40](e28ea40)) * fix backend CI ([448e823](448e823)) * fix backend CI ([bb89273](bb89273)) * fix CD ([f128779](f128779)) * fix CI ([2a7761d](2a7761d)) * Fix comprehensive Checkov security violations across all Terraform modules ([c635805](c635805)) * fix gitops env ([5658004](5658004)) * fix gitops env ([c27a240](c27a240)) * fix gitops env ([1d79c40](1d79c40)) * fix gitops env ([71a5a3e](71a5a3e)) * fix gitops env ([61a203c](61a203c)) * fix gitops env ([4988815](4988815)) * fix postgre auth ([5fa826d](5fa826d)) * fix postgre auth ([e02fbf2](e02fbf2)) * fix startup ([0574335](0574335)) * fix workflow env ([5080d6f](5080d6f)) * fixe backend test ci ([d54db06](d54db06)) * fixe backend test ci ([b93bfd8](b93bfd8)) * fixe front/back test ci ([944297d](944297d)) * fixe front/back test ci ([6ef63c2](6ef63c2)) * fixe front/back test ci ([b9e06c0](b9e06c0)) * fixe front/back test ci ([61e6b14](61e6b14)) * Initial commit ([111b8bc](111b8bc)) * Merge pull request #1 from YilingCAI/feature/cai ([adf3f7d](adf3f7d)), closes [#1](#1) * Merge pull request #18 from YilingCAI/feature/cai ([c1d2494](c1d2494)), closes [#18](#18) * Merge pull request #20 from YilingCAI/feature/cai ([efa291b](efa291b)), closes [#20](#20) * Merge pull request #21 from YilingCAI/feature/cai ([c5cf449](c5cf449)), closes [#21](#21) * Merge pull request #22 from YilingCAI/feature/cai ([f8de3a7](f8de3a7)), closes [#22](#22) * Merge pull request #23 from YilingCAI/feature/cai ([c4b5fc0](c4b5fc0)), closes [#23](#23) * Merge pull request #24 from YilingCAI/feature/cai ([b36e538](b36e538)), closes [#24](#24) * Merge pull request #3 from YilingCAI/feature/cai ([6f588e7](6f588e7)), closes [#3](#3) * Merge pull request #4 from YilingCAI/feature/cai ([7ef92f2](7ef92f2)), closes [#4](#4) * Merge pull request #6 from YilingCAI/feature/cai ([f613a01](f613a01)), closes [#6](#6) * Merge pull request #7 from YilingCAI/developer ([4237b79](4237b79)), closes [#7](#7) * modify backend README comments ([0739fe5](0739fe5)) * modify CI ([c629cf5](c629cf5)) * modify frontend README comments ([6373ca7](6373ca7)) * move create access token fonction to security.py ([e00ea2f](e00ea2f)) * new feature branch for testing CI ([09454fc](09454fc)) * new project settup ([737d5b2](737d5b2)) * refactor github actions ([81e93c3](81e93c3)) * refactor logging, devops CICD using shell scritps ([f8a5f51](f8a5f51)) * set up project structure ([4b45a5b](4b45a5b)) * setup ([75ba591](75ba591)) * setup ([5a63c09](5a63c09)) * split infra and code base ([5750338](5750338)) * test ci ([cc74cc5](cc74cc5)) * update Dockerfile backend ([e8833d5](e8833d5)) * fix(backend): add sslmode=require for RDS SSL connections ([431247d](431247d)) * fix(cd): run yq container as root to fix file write permissions ([9b847b5](9b847b5)) * fix(ci): hardcode postgres credentials and port in service container ([f9b7f0a](f9b7f0a)) * fix(frontend): use nginx-unprivileged to avoid chown permission error in K8s ([4f7e1cc](4f7e1cc)) * fix(frontend): use relative API base URL instead of localhost:8000 ([5602ad2](5602ad2)) * feat(cd): sync k8s DB secret from GitHub Secrets before ArgoCD sync ([4e604b9](4e604b9)) * feature: terraform dev valide ([c7dda29](c7dda29))
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
test CI event PR to develop