Staging#160
Merged
Merged
Conversation
chore(infra): prepare kubernetes manifests and ci pipeline for production
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughCoordinated production-readiness updates: instrument the health endpoint, configure Kubernetes metrics-server, tune HPA (CPU 60%, memory 80% and behavior), update CI to tag/push both ChangesProduction readiness: monitoring, scaling, and deployment
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Deploy Preview for system-craft-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: 4
🧹 Nitpick comments (4)
app/api/health/route.ts (1)
4-6: 💤 Low valueImplementation correctly follows the
withMetricscontract.The
reqparameter is required by theAppRouteHandlertype signature (as defined insrc/lib/withMetrics.ts) even though it's not used in this handler's body. The static analysis warning about the unused parameter is a false positive in this case.If you prefer to silence the linter warning, you can prefix the parameter with an underscore:
Optional: Silence the linter warning
-export const GET = withMetrics('/api/health', async (req: NextRequest) => { +export const GET = withMetrics('/api/health', async (_req: NextRequest) => {🤖 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 `@app/api/health/route.ts` around lines 4 - 6, The GET handler's unused req parameter triggers a lint warning even though withMetrics requires it; update the function signature for export const GET = withMetrics('/api/health', async (req: NextRequest) => { ... }) to rename the parameter to _req (or _request) to silence the linter while keeping the AppRouteHandler contract intact—locate the GET export and the withMetrics wrapper to make this change.tests/load/k6-spike.js (1)
22-26: 💤 Low valueThe anonymous default export is correct k6 convention.
The static analysis warning about "Unexpected default export of anonymous function" is a false positive. k6 requires the virtual user (VU) code to be exported as a default function, and it's standard practice to use an anonymous function for this purpose.
However, consider adding a check for the response body to validate the endpoint returns the expected
{ status: 'ok' }payload:Optional: Add response body validation
export default function () { const res = http.get(`${BASE_URL}/api/health`); - check(res, { 'status 200': (r) => r.status === 200 }); + check(res, { + 'status 200': (r) => r.status === 200, + 'body is ok': (r) => r.json('status') === 'ok' + }); // No Sleep to maximize load }🤖 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 `@tests/load/k6-spike.js` around lines 22 - 26, The anonymous default export is fine for k6; update the health check in the exported function to also validate the JSON body from the GET to `${BASE_URL}/api/health`: after calling http.get and the existing check on r.status, parse the response body (use res.json() or JSON.parse(res.body)) and add a check that the parsed object's status equals 'ok' (augment the check call or add a new check), referencing the exported default function, http.get, check, BASE_URL, and the '/api/health' endpoint..github/workflows/ci.yml (2)
124-126: ⚡ Quick winOptional defense-in-depth: move image ref construction out of inline
${{ ... }}inrun:
- The usual script-injection risk from
${{ ... }}inrun:applies most when values are untrusted; hereenv.REPOis derived fromGITHUB_REPOSITORYandgithub.shais a hex SHA, and the push step only runs for non-pull_requestevents.- Defense-in-depth (and to avoid static-analyzer noise): set the image repo/tag in step
env:and reference them inrun:.🔒 Proposed defense-in-depth hardening
- name: Push Docker image if: github.event_name != 'pull_request' + env: + IMAGE_REPO: ghcr.io/${{ env.REPO }}/systemcraft-web + IMAGE_TAG: ${{ github.sha }} run: | - docker push ghcr.io/${{ env.REPO }}/systemcraft-web:latest - docker push ghcr.io/${{ env.REPO }}/systemcraft-web:${{ github.sha }} + docker push ${IMAGE_REPO}:latest + docker push ${IMAGE_REPO}:${IMAGE_TAG}🤖 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 124 - 126, Move the dynamic image references out of the inline run script and into the step environment to avoid `${{ ... }}` expansion inside run; specifically, in the CI step that currently calls docker push with ghcr.io/${{ env.REPO }}/systemcraft-web:latest and ghcr.io/${{ env.REPO }}/systemcraft-web:${{ github.sha }}, add step-level env variables (e.g., IMAGE_LATEST and IMAGE_SHA using expressions for env.REPO and github.sha) and then reference those env vars inside the run block (docker push $IMAGE_LATEST and docker push $IMAGE_SHA) so the run script uses plain env-vars instead of `${{ ... }}` interpolation.
160-164: Deploy step is a placeholder with good preparation.The expanded deploy step with commented kubectl examples provides a clear path forward for actual deployment implementation. The approach of using commit SHA tags (line 163) aligns with production best practices for reproducibility.
The commented examples are syntactically correct—GitHub Actions will expand
${{ github.sha }}before shell execution. When you're ready to implement actual deployment, consider:
- Adding kubectl authentication setup
- Implementing deployment verification checks
- Adding rollback capability
Would you like me to generate a complete deployment implementation or open an issue to track this task?
🤖 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 160 - 164, The deploy step currently only echoes a message and leaves kubectl/sed commands commented out; replace this placeholder with a real deploy sequence that (1) authenticates to the Kubernetes cluster (e.g., setup kubeconfig or use cloud provider login action), (2) tags the image reference systemcraft-web:${{ github.sha }} in your manifests (the current sed example targeting kubernetes/deployment.yaml is fine as the replacement approach), (3) applies the manifests via kubectl apply, and (4) runs post-deploy verification checks (kubectl rollout status, health probes) and a simple rollback path (kubectl rollout undo) on failure; ensure secrets/credentials are provided via GitHub Actions secrets and any cloud-provider-specific auth actions are added before invoking kubectl.
🤖 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 `@kubernetes/hpa.yaml`:
- Line 20: The current HPA config sets averageUtilization: 40 which is overly
aggressive given the CPU request of 250m; update the averageUtilization value in
the HorizontalPodAutoscaler spec (the averageUtilization field) to a more
conservative value (suggest 60 or 70) or confirm 40 with load-test evidence,
then adjust the threshold accordingly so scaling triggers align with expected
pod CPU usage and reduce unnecessary pod churn.
- Around line 27-33: The HPA's scaleUp is too aggressive: update
behavior.scaleUp.stabilizationWindowSeconds from 0 to a sane value (e.g., 30 or
60) and reduce the scale-up policy to add fewer pods less frequently by changing
the policy under behavior.scaleUp.policies (the entry with type: Pods) from
value: 4 and periodSeconds: 15 to something like value: 2 and periodSeconds: 30;
adjust these numbers as needed for your app's startup time and traffic patterns
to avoid thrashing.
In `@kubernetes/metrics-server-patch.yaml`:
- Line 12: Remove the --kubelet-insecure-tls flag from the metrics-server
manifest (the argument currently shown as "--kubelet-insecure-tls") and instead
configure metrics-server to verify kubelet TLS: either ensure kubelets present
valid certificates trusted by the cluster CA or add the
--kubelet-certificate-authority argument pointing to a mounted CA bundle (create
a Secret/ConfigMap and mount it into the pod if needed) so production uses
proper certificate verification rather than disabling TLS.
In `@tests/load/k6-spike.js`:
- Line 4: The import includes an unused symbol `sleep`; remove `sleep` from the
import statement so it only imports `check` (i.e., change `import { sleep, check
} from 'k6';` to `import { check } from 'k6';`) to reflect the intentional
no-sleep behavior mentioned in the comment.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 124-126: Move the dynamic image references out of the inline run
script and into the step environment to avoid `${{ ... }}` expansion inside run;
specifically, in the CI step that currently calls docker push with ghcr.io/${{
env.REPO }}/systemcraft-web:latest and ghcr.io/${{ env.REPO
}}/systemcraft-web:${{ github.sha }}, add step-level env variables (e.g.,
IMAGE_LATEST and IMAGE_SHA using expressions for env.REPO and github.sha) and
then reference those env vars inside the run block (docker push $IMAGE_LATEST
and docker push $IMAGE_SHA) so the run script uses plain env-vars instead of
`${{ ... }}` interpolation.
- Around line 160-164: The deploy step currently only echoes a message and
leaves kubectl/sed commands commented out; replace this placeholder with a real
deploy sequence that (1) authenticates to the Kubernetes cluster (e.g., setup
kubeconfig or use cloud provider login action), (2) tags the image reference
systemcraft-web:${{ github.sha }} in your manifests (the current sed example
targeting kubernetes/deployment.yaml is fine as the replacement approach), (3)
applies the manifests via kubectl apply, and (4) runs post-deploy verification
checks (kubectl rollout status, health probes) and a simple rollback path
(kubectl rollout undo) on failure; ensure secrets/credentials are provided via
GitHub Actions secrets and any cloud-provider-specific auth actions are added
before invoking kubectl.
In `@app/api/health/route.ts`:
- Around line 4-6: The GET handler's unused req parameter triggers a lint
warning even though withMetrics requires it; update the function signature for
export const GET = withMetrics('/api/health', async (req: NextRequest) => { ...
}) to rename the parameter to _req (or _request) to silence the linter while
keeping the AppRouteHandler contract intact—locate the GET export and the
withMetrics wrapper to make this change.
In `@tests/load/k6-spike.js`:
- Around line 22-26: The anonymous default export is fine for k6; update the
health check in the exported function to also validate the JSON body from the
GET to `${BASE_URL}/api/health`: after calling http.get and the existing check
on r.status, parse the response body (use res.json() or JSON.parse(res.body))
and add a check that the parsed object's status equals 'ok' (augment the check
call or add a new check), referencing the exported default function, http.get,
check, BASE_URL, and the '/api/health' endpoint.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb3aa5b1-1268-4976-8500-69d89f67f1be
📒 Files selected for processing (6)
.github/workflows/ci.ymlapp/api/health/route.tskubernetes/hpa.yamlkubernetes/metrics-server-patch.yamlmonitoring/prometheus/prometheus.ymltests/load/k6-spike.js
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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
1 participant
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 by CodeRabbit