Proj i04 7/clean up cicd pipeline - #133
Merged
Merged
Conversation
The wake step's grep only recognized 'Stopped'; any other cluster.sh output (transitional states, garbled az errors) silently proceeded into a confusing Terraform failure. Allowlist the script's actual outputs and fail loudly otherwise. Prod dispatch defaulted to the mutable 'latest' tag, making an accidental deploy of unreleased main a single click. Still deployable, but only by typing it deliberately.
Dev and prod each carried their own copy of the Ansible toolchain setup and playbook invocation, and they had already drifted (tag handling, bootstrap args). Both workflows now call .github/actions/ansible-deploy, which runs bootstrap (explicit opt-in flag, dev only) + deploy, pins ansible-core for reproducible runs, smoke-checks the public URL with strict TLS after deploying, and writes a step summary (inventory, tag, URL) to the run page. Trigger-specific tag derivation stays in the callers as dedicated steps.
Eight per-service workflows were ~95% copy-paste and had already drifted (checkout versions, persist-credentials, permissions), and every PR ran all of them regardless of what changed. One ci.yml now detects changed components (dorny/paths-filter), runs only the affected jobs (java services as a dynamic matrix; a spec change fans out to everything since DTOs are generated from it), and funnels results into a ci-ok aggregator gate. Branch protection should require exactly one check: ci-ok — dynamic matrix legs must never be individually required (an ungenerated leg reports nothing and deadlocks the PR). Also: stale PR runs are now cancelled on new pushes, every job has a timeout, and main gets a post-merge CI run.
Contributor
|
Important Review skippedToo many files! This PR contains 162 files, which is 112 over the limit of 50. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (162)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a reusable Ansible deployment action, updates development and production deployment workflows to use it, and consolidates component-specific CI checks into one path-aware workflow with an aggregate status gate. ChangesAnsible deployment workflow
Consolidated component CI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant ChangesJob
participant ComponentJobs
participant CIOK
GitHub->>ChangesJob: detect changed components
ChangesJob->>ComponentJobs: enable matching validation jobs
ComponentJobs->>CIOK: report job results
CIOK->>GitHub: publish aggregate CI status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/ci.yml (3)
156-160: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEnable caching for Python dependencies.
Consider using the built-in caching for
pipinactions/setup-pythonto speed up the installation step for the email service.♻️ Proposed refactor
- name: Set up Python 3.12 uses: actions/setup-python@v6 with: python-version: "3.12" + cache: 'pip' + cache-dependency-path: 'services/email/requirements.txt'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 156 - 160, Update the “Set up Python 3.12” actions/setup-python step to enable its built-in pip dependency caching by configuring the cache option, while preserving the existing Python version and setup behavior.
122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify Maven caching.
You can leverage the built-in caching provided by
actions/setup-java, which automatically handles the~/.m2/repositorycaching based onpom.xmlfiles. This removes the need for a separateactions/cachestep.♻️ Proposed refactor
- name: Set up Java 21 uses: actions/setup-java@v5 with: java-version: 21 distribution: temurin + cache: maven - - - name: Cache Maven repository - uses: actions/cache@v6 - with: - path: ~/.m2/repository - key: maven-${{ hashFiles(format('services/{0}/pom.xml', matrix.service)) }} - restore-keys: maven-🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 122 - 134, Remove the separate “Cache Maven repository” actions/cache step and configure the existing “Set up Java 21” actions/setup-java step to enable Maven dependency caching via its built-in cache option. Preserve the current Java version and Temurin distribution settings.
17-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid cancelling CI runs on the
mainbranch.Currently,
cancel-in-progress: truewill cancel running CI jobs on themainbranch if a newer commit is pushed. While this is desirable for pull requests, cancelling jobs onmainmay result in missed deployments or missing status checks for intermediate commits.Consider conditionally disabling cancellation for pushes to the default branch.
♻️ Proposed refactor
concurrency: group: ci-${{ github.event.pull_request.number || github.ref }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 17 - 20, Update the workflow concurrency configuration so cancel-in-progress remains enabled for pull request runs but is disabled for pushes to the main/default branch. Preserve the existing concurrency group behavior and use the workflow’s GitHub event/ref context to distinguish pull requests from default-branch pushes..github/actions/ansible-deploy/action.yml (1)
47-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the
kubernetespip package too.The comment explains ansible-core is pinned so a "deploy never breaks because pip resolved a different ansible-core that morning" — the same risk applies to the unpinned
kubernetespackage, which thekubernetes.corecollection depends on.♻️ Pin kubernetes package
- pip install 'ansible-core==2.17.*' kubernetes + pip install 'ansible-core==2.17.*' 'kubernetes==31.*'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/ansible-deploy/action.yml around lines 47 - 53, Update the “Install Ansible + collections” step so the pip-installed kubernetes package is pinned to an intentional compatible version, alongside the existing ansible-core constraint. Preserve the collection installation command and extend the nearby comment to indicate that both dependency versions must be deliberately bumped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/ansible-deploy/action.yml:
- Around line 63-117: The reusable action still interpolates dynamic inputs
directly into shell scripts. In .github/actions/ansible-deploy/action.yml lines
63-117, add step-level env mappings for the listed vault, inventory, deployment,
URL, and extra-argument inputs, then reference those variables with shell-safe
quoting in the Bootstrap platform, Deploy app, Smoke check, and Write deploy
summary steps. In .github/workflows/cd-dev.yml lines 96-107, apply the same
pattern in Compute image tag by mapping github.event.workflow_run.head_sha and
inputs.image_tag to env variables and referencing them as shell variables; use
cd-prod.yml’s Normalize image tag step as the pattern.
In @.github/workflows/ci.yml:
- Around line 47-74: Update the paths-filter definitions in the filters block to
avoid list-item anchor references that create nested arrays. Remove the shared
anchor usage and repeat api/openapi.yaml and .github/workflows/ci.yml as flat
entries in each affected filter, preserving each service-specific path and the
existing openapi filter.
---
Nitpick comments:
In @.github/actions/ansible-deploy/action.yml:
- Around line 47-53: Update the “Install Ansible + collections” step so the
pip-installed kubernetes package is pinned to an intentional compatible version,
alongside the existing ansible-core constraint. Preserve the collection
installation command and extend the nearby comment to indicate that both
dependency versions must be deliberately bumped.
In @.github/workflows/ci.yml:
- Around line 156-160: Update the “Set up Python 3.12” actions/setup-python step
to enable its built-in pip dependency caching by configuring the cache option,
while preserving the existing Python version and setup behavior.
- Around line 122-134: Remove the separate “Cache Maven repository”
actions/cache step and configure the existing “Set up Java 21”
actions/setup-java step to enable Maven dependency caching via its built-in
cache option. Preserve the current Java version and Temurin distribution
settings.
- Around line 17-20: Update the workflow concurrency configuration so
cancel-in-progress remains enabled for pull request runs but is disabled for
pushes to the main/default branch. Preserve the existing concurrency group
behavior and use the workflow’s GitHub event/ref context to distinguish pull
requests from default-branch pushes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a0c3af9-da23-477b-82f2-27b7a86b1316
📒 Files selected for processing (12)
.github/actions/ansible-deploy/action.yml.github/workflows/cd-dev.yml.github/workflows/cd-prod.yml.github/workflows/ci-application.yml.github/workflows/ci-auth.yml.github/workflows/ci-document.yml.github/workflows/ci-email.yml.github/workflows/ci-gateway.yml.github/workflows/ci-genai.yml.github/workflows/ci-openapi.yml.github/workflows/ci-web-client.yml.github/workflows/ci.yml
💤 Files with no reviewable changes (8)
- .github/workflows/ci-gateway.yml
- .github/workflows/ci-email.yml
- .github/workflows/ci-application.yml
- .github/workflows/ci-web-client.yml
- .github/workflows/ci-genai.yml
- .github/workflows/ci-auth.yml
- .github/workflows/ci-document.yml
- .github/workflows/ci-openapi.yml
Chain the pipeline CI -> build-images (workflow_run, success only) -> cd-dev, so a red main can never publish or deploy images. Pin checkout and sha- tags to the CI run's head_sha to survive rapid merges. Drop the v* tag trigger from build-images: a git tag can point at any commit, so tag-triggered builds bypassed the CI gate. Releases now promote via crane copy in cd-prod (sha-<short> -> semver), so prod ships the exact bytes dev ran and deploy structurally waits on the tags existing (needs: promote-images) instead of relying on approval timing.
Spotless (palantir-java-format) + slim shared Checkstyle for the four Spring services, a repo-root ruff.toml both Python services extend, mypy (permissive baseline) for genai/email, and Prettier plus the missing eslint.config.js for web-client (npm run lint errored since ESLint 9 with no flat config). Generated OpenAPI code is excluded in each tool's own config so every entry point agrees.
Mechanical palantir-java-format run — no manual edits. SHA recorded in .git-blame-ignore-revs at the end of this branch.
Mechanical ruff format run — no manual edits. SHA recorded in .git-blame-ignore-revs at the end of this branch.
email previously ran ruff defaults; the shared root config adds B/UP/SIM. Mostly auto-fixes (datetime.UTC alias, contextlib.suppress). The fastapi.Depends B008 whitelist moves from genai's config to the root — it is a FastAPI idiom, not a genai quirk.
Test classes deliberately use the method_scenario_expectation underscore convention; Checkstyle's MethodName now skips src/test.
Mechanical prettier --write run — no manual edits. SHA recorded in .git-blame-ignore-revs at the end of this branch.
Permissive-baseline mypy (now part of CI) surfaced real gaps: unguarded fetchone()/pool access, a get_messages signature that hid its None return, SecretStr coercion for langchain api keys, and an untyped trace_config that every ainvoke call site tripped over.
Local repo hooks call the same project entry points CI runs (uv-pinned ruff, web-client npm scripts) so tool versions stay single-sourced. Java tooling and mypy stay CI-only — too slow for commit time. hadolint is CI-only too: its pre-commit hook needs docker on every dev machine.
Python jobs gain ruff format --check + mypy; web-client gains eslint + prettier --check (lint existed as a script but never ran in CI — and was broken locally, see eslint.config.js commit). Java needs no new steps: Spotless and Checkstyle are bound into mvnw verify. New path-filtered infra-lint job lints workflows (actionlint via the pre-commit hook, so the pin lives once) and Dockerfiles (hadolint).
One-time trailing-whitespace/end-of-file sweep so hooks don't trip on pre-existing files. The fixer hooks now exclude OpenAPI codegen output — fixing it up would make it drift from what codegen produces.
Go template syntax is not valid YAML; helm lint owns those files.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .oauth2ResourceServer(oauth2 -> oauth2 | ||
| .bearerTokenResolver(cookieOrHeaderTokenResolver()) | ||
| .jwt(Customizer.withDefaults())); | ||
| http.csrf(csrf -> csrf.disable()) |
Collaborator
Author
There was a problem hiding this comment.
Will address in a later ticket
| .oauth2ResourceServer(oauth2 -> oauth2 | ||
| .bearerTokenResolver(cookieOrHeaderTokenResolver()) | ||
| .jwt(Customizer.withDefaults())); | ||
| http.csrf(csrf -> csrf.disable()) |
| .oauth2ResourceServer(oauth2 -> oauth2 | ||
| .bearerTokenResolver(cookieOrHeaderTokenResolver()) | ||
| .jwt(Customizer.withDefaults())); | ||
| http.csrf(csrf -> csrf.disable()) |
| .with(jwt().jwt(j -> j.subject(userId.toString()))) | ||
| .contentType("application/json") | ||
| .content("{\"company\":\"\",\"job_title\":\"Engineer\"}")) | ||
| .andExpect(status().isUnprocessableEntity()) |
georges-nasrallah
added a commit
that referenced
this pull request
Jul 16, 2026
…eneration Resolve conflicts from the repo-wide formatter rollout (#133). The genai conflicts were semantic, not cosmetic: - chat.py/summarizer: keep SUMMARY_EVERY and HISTORY_WINDOW as separate constants; main still conflated compression cadence with the replay window. - chain.py: keep the four-argument system prompt — main's two-argument call would KeyError against the merged prompt's placeholders. - session.py: keep is_first_user_session removed; the profile is now injected every turn, so the first-session check is dead code. - DocumentServiceImplTest: application_id is optional in the spec, so the generated DTO has no three-argument constructor. web-client conflicts were formatter-only; resolved to this branch's logic and reformatted with the newly adopted Prettier config.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #
Motivation & Context
Changes
Steps for Testing
Checklist
.env.examplemake -C api generate) if any endpoint changedSummary by CodeRabbit
New Features
Improvements
Chores