feat: add promtool check to preview - #43
Conversation
|
Preview environment torn down. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds promtool to the runtime image, validates rendered PrometheusRule resources during render, shows the check result in the preview UI, and expands integration tests for success, failure, and missing-binary cases. ChangesPromtool Validation Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RenderRoute
participant Helm
participant Promtool
Client->>RenderRoute: POST /api/v2/render/:chart/:deployment
RenderRoute->>Helm: helm dependency update
RenderRoute->>Helm: helm template
Helm-->>RenderRoute: rendered YAML output
RenderRoute->>RenderRoute: extractPrometheusRuleGroups(output)
RenderRoute->>Promtool: promtool check rules (temp rules.yaml)
Promtool-->>RenderRoute: check result (passed/errors/output)
RenderRoute-->>Client: { ok, output, check }
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
13-25: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRun the runtime image as a non-root user.
The final stage still starts as root. This PR adds more file/process activity (
helm,promtool, temp files) to the request path, so any server-side compromise inherits full container privileges. Drop toUSER node(or a dedicated user) after the installs/copies and fix ownership on/appas needed.Possible hardening change
FROM node:22-alpine RUN apk add --no-cache git helm WORKDIR /app @@ COPY server/ ./server/ COPY sample/ ./sample/ COPY index.html ./ +RUN chown -R node:node /app + +USER node EXPOSE 8080 ENV PORT=8080 CMD ["node", "server.js"]🤖 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 `@Dockerfile` around lines 13 - 25, The final runtime stage in the Dockerfile still runs as root; update the image to drop privileges after the package installs and copy steps by switching to a non-root user such as node. Make sure the /app workdir and any copied artifacts used by the app (including dist and promtool) have the correct ownership/permissions before the USER change so the container can run normally without root.Source: Linters/SAST tools
🤖 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 `@server/routes/render.js`:
- Line 118: The preview path in render.js is mutating the live chart checkout
because runCommand(helm, ['dependency', 'build', templateDir], ...) writes into
req.gitopsDir. Update the Helm flow in the preview handler so it operates on a
temporary copy of the chart or uses a prebuilt dependency directory instead of
templateDir from the live repo. Keep the fix localized around the preview
command path in render.js and preserve the read-only behavior of the request.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 13-25: The final runtime stage in the Dockerfile still runs as
root; update the image to drop privileges after the package installs and copy
steps by switching to a non-root user such as node. Make sure the /app workdir
and any copied artifacts used by the app (including dist and promtool) have the
correct ownership/permissions before the USER change so the container can run
normally without root.
🪄 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: 43780962-101b-47df-bede-7ebbe33b7768
📒 Files selected for processing (4)
Dockerfileserver/routes/render.jssrc/pages/AlertUserView.jsxtests/integration/render-api.test.js
a8bf2d9 to
1cfb92d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/routes/render.js (1)
35-52: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftFull recursive repo copy on every preview request.
copyGitopsDirToTempnow correctly resolves the previously-flagged issue ofhelm dependency buildmutating the live checkout — good fix. However, it does a full recursivefs.cpof the entirereq.gitopsDir(minus.git/node_modules/__pycache__) on every render/preview call, then discards it. For a gitops repo with many charts/deployments this is an expensive I/O operation on a hot, frequently-invoked path, and cost scales with total repo size rather than with the size of the specific chart being previewed.Consider narrowing the copy to just the chart/deployment subtree actually needed (
chartDir/templateDirplus any localfile://chart dependencies), or caching/reusing a synced working copy instead of a fresh full copy per request.Also applies to: 128-150
🤖 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 `@server/routes/render.js` around lines 35 - 52, The full recursive repo copy in copyGitopsDirToTemp is too expensive for the render/preview hot path because it clones the entire gitops checkout on every request. Update the render flow in server/routes/render.js to avoid fs.cp of req.gitopsDir wholesale; instead copy only the chart/deployment subtree needed by the preview (the chartDir/templateDir and any local file:// dependencies), or reuse a cached synced working copy across requests. Keep copyGitopsDirToTemp focused on the minimal needed workspace and ensure the callers still receive a valid copiedGitopsDir.src/pages/AlertUserView.jsx (1)
369-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the promtool output height like the YAML preview.
The main rendered-YAML
<pre>below (Line 382-388) hasmaxHeight: 500, overflow: 'auto', but the promtool output<pre>inside theAlertdescription has no height bound. Largepromtooloutput could push the modal well beyond the viewport.💚 Suggested fix
previewCheck.output - ? <pre style={{ margin: 0, whiteSpace: 'pre-wrap', wordBreak: 'break-word' }}>{previewCheck.output}</pre> + ? <pre style={{ margin: 0, whiteSpace: 'pre-wrap', wordBreak: 'break-word', maxHeight: 200, overflow: 'auto' }}>{previewCheck.output}</pre> : null🤖 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 `@src/pages/AlertUserView.jsx` around lines 369 - 381, The promtool output rendered in the AlertUserView preview is unbounded and can expand the modal too far. Update the previewCheck Alert description’s <pre> block to match the rendered YAML preview behavior by adding a max height and scroll overflow so large promtool output stays contained. Use the existing previewCheck conditional render in AlertUserView and keep the same word-wrapping behavior while bounding the display area.
🤖 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.
Nitpick comments:
In `@server/routes/render.js`:
- Around line 35-52: The full recursive repo copy in copyGitopsDirToTemp is too
expensive for the render/preview hot path because it clones the entire gitops
checkout on every request. Update the render flow in server/routes/render.js to
avoid fs.cp of req.gitopsDir wholesale; instead copy only the chart/deployment
subtree needed by the preview (the chartDir/templateDir and any local file://
dependencies), or reuse a cached synced working copy across requests. Keep
copyGitopsDirToTemp focused on the minimal needed workspace and ensure the
callers still receive a valid copiedGitopsDir.
In `@src/pages/AlertUserView.jsx`:
- Around line 369-381: The promtool output rendered in the AlertUserView preview
is unbounded and can expand the modal too far. Update the previewCheck Alert
description’s <pre> block to match the rendered YAML preview behavior by adding
a max height and scroll overflow so large promtool output stays contained. Use
the existing previewCheck conditional render in AlertUserView and keep the same
word-wrapping behavior while bounding the display area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a447e623-aa28-4269-b9a6-2948d5d10a9c
📒 Files selected for processing (4)
Dockerfileserver/routes/render.jssrc/pages/AlertUserView.jsxtests/integration/render-api.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
The full gitops copy added for CodeRabbit's mutation finding re-litigated a decision this project already made: #29 raised the same pollution problem, PR #30's temp-dir mirror was closed as over-engineered, and PR #31 settled it — the gitops repo's own .gitignore keeps Chart.lock and charts/*.tgz out of version control (doc/gitops-repo-setup.md). Copying the whole repo per render also scaled preview latency with repo size and was the root cause of the flaky e2e preview timeout on CI. Switching build → update fixes a latent bug that predates this PR: helm dependency build hard-errors on a stale on-disk Chart.lock ("lock file out of sync") as soon as a chart version is bumped, breaking Preview for every previously-rendered deployment. update re-resolves each time — the lock has no pinning value for same-repo file:// dependencies — and prunes outdated .tgz files as a side effect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/routes/render.js`:
- Around line 128-132: The `helm template` invocation in `render.js` can exceed
the default `execFile` buffer and fail on large charts. Update the
`runCommand(helm, templateArgs, ...)` call to pass a larger `maxBuffer` so
`stdout` can be captured reliably; if `checkPrometheusRules` can also emit large
output through `runCommand`, apply the same buffer increase there as well.
🪄 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: f484e4e7-f536-42b2-96f4-5265643b274a
📒 Files selected for processing (3)
server/routes/render.jssrc/pages/AlertUserView.jsxtests/integration/render-api.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/AlertUserView.jsx
rophy
left a comment
rophy
left a comment
There was a problem hiding this comment.
Clean, well-tested PR. Promtool integration is solid — good error handling, proper temp-dir cleanup, and the fake-helm/fake-promtool test setup is well done.
execFile's default maxBuffer is 1MiB, so a large chart render or rule
set could fail with ERR_CHILD_PROCESS_STDIO_MAXBUFFER and turn Preview
into { ok: false } instead of returning output.
Addresses CodeRabbit review on #43.
|
Fixed in cbdb4f3 — raised |
Bump version from 1.3.0 to 1.4.0 to trigger a new image build and GitHub Release via CI. Since 1.3.0: deployment sync (#44), alert overview mode with column filters (#37), promtool validation (#41, #43), plus several fixes — enough new features to warrant a minor bump per this project's convention (1.2.0 -> 1.3.0 similarly bumped minor for the Gitea migration).
2 participants
Summary
Refs #38
Adds promtool validation to the Preview flow so rendered PrometheusRule YAML is checked before it is shown to the user.
PrometheusRule.spec.groupspromtool check rulesfrom the server-side render routepromtoolin the runtime Docker image from a pinned Prometheus imageBehavior
promtool check rulesValidation
npm run lintnpm run buildnpm test -- tests/integration/render-api.test.jsNotes
Full
npm teststill has one existing locale-sensitive failure unrelated to this change:tests/unit/git-lib.test.jsOn branch位於分支 masterSummary by CodeRabbit
PrometheusRuleYAML via Promtool and show clear pass/fail/skipped status.