feat: Add helm chart for QueryFlux#75
Open
themoah wants to merge 3 commits into
Open
Conversation
…Secret mounts config.yaml from a Secret
|
Warning Review limit reached
More reviews will be available in 23 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a complete, production-ready Helm chart for QueryFlux. It adds CI validation via a new ChangesQueryFlux Helm Chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
themoah
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
scripts/check-helm-chart.sh (1)
54-57: 💤 Low valueConsider refactoring the boolean logic for clarity.
The
A && B || Cconstruct on lines 55-57 is functionally correct but triggers a Shellcheck info notice because this pattern can be confusing (it's not equivalent to if-then-else in all contexts).♻️ Clearer alternative
-# Admin Secret must use configurable key names rather than hardcoded ones. -grep -q '{{ .Values.existingSecret.usernameKey }}' "$CHART_DIR/templates/secret.yaml" \ - && grep -q '{{ .Values.existingSecret.passwordKey }}' "$CHART_DIR/templates/secret.yaml" \ - || fail "templates/secret.yaml must use configurable admin Secret key names" +# Admin Secret must use configurable key names rather than hardcoded ones. +grep -q '{{ .Values.existingSecret.usernameKey }}' "$CHART_DIR/templates/secret.yaml" \ + || fail "templates/secret.yaml must use configurable usernameKey" +grep -q '{{ .Values.existingSecret.passwordKey }}' "$CHART_DIR/templates/secret.yaml" \ + || fail "templates/secret.yaml must use configurable passwordKey"This makes each check independent and easier to understand.
🤖 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 `@scripts/check-helm-chart.sh` around lines 54 - 57, The boolean chain using "grep -q ... && grep -q ... || fail" is confusing and triggers shellcheck; replace it with explicit, independent checks so each grep result is evaluated clearly and failure is invoked only if either check misses: run grep -q for '{{ .Values.existingSecret.usernameKey }}' against "$CHART_DIR/templates/secret.yaml" and separately for '{{ .Values.existingSecret.passwordKey }}', and call the existing fail function (referenced as fail) if either check fails; this keeps CHART_DIR and templates/secret.yaml references and makes the intent explicit and shellcheck-compliant.charts/queryflux/templates/servicemonitor.yaml (1)
25-26: ⚡ Quick winConsider validating the scrapeTimeout vs interval relationship.
Prometheus requires
scrapeTimeoutto be less thaninterval. If misconfigured (scrapeTimeout >= interval), Prometheus may log warnings or fail to scrape metrics properly. Adding a template validation would catch this at install time rather than runtime.🛡️ Proposed validation check
Add this check near the top of the template:
{{- if .Values.serviceMonitor.enabled }} +{{- if not (lt (int .Values.serviceMonitor.scrapeTimeout) (int .Values.serviceMonitor.interval)) }} +{{- fail "serviceMonitor.scrapeTimeout must be less than serviceMonitor.interval" }} +{{- end }} apiVersion: monitoring.coreos.com/v1Note: This assumes the values are numeric strings (e.g., "30s", "60s"). If they're already integers, the validation logic may need adjustment.
🤖 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 `@charts/queryflux/templates/servicemonitor.yaml` around lines 25 - 26, Prometheus requires scrapeTimeout < interval, but the template uses .Values.serviceMonitor.interval and .Values.serviceMonitor.scrapeTimeout without validation; add a Helm template pre-check that parses both values to numeric seconds (e.g., strip trailing "s" with replace and convert with atoi) and call fail (Helm's fail function) if scrapeTimeout >= interval so installation fails early; implement this check near the top of the servicemonitor.yaml template referencing .Values.serviceMonitor.interval and .Values.serviceMonitor.scrapeTimeout and include a clear error message indicating the offending values.charts/queryflux/examples/external-config-values.yaml (1)
19-26: 💤 Low valueSimplify disabled port configuration.
When
studio.enabledisfalse, the port details (name,port,targetPort,protocol) are not used by the Helm templates. Consider removing the redundant fields for clarity:♻️ Suggested simplification
service: ports: studio: enabled: false - name: studio - port: 3000 - targetPort: 3000 - protocol: TCP🤖 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 `@charts/queryflux/examples/external-config-values.yaml` around lines 19 - 26, The service.ports.studio block contains redundant fields when studio.enabled is false; remove the nested keys name, port, targetPort and protocol under service.ports.studio and keep only enabled: false to simplify the values file and avoid confusion with unused configuration (look for the service.ports.studio entry and the studio.enabled flag to make the change).charts/queryflux/examples/production-values.yaml (1)
28-31: ⚡ Quick winUse placeholder credentials in production example.
Although the comment clearly states "Demo value only," having literal credentials (
postgres://queryflux:queryflux@...) in a file namedproduction-values.yamlcreates a copy-paste risk. Consider replacing with explicit placeholders to prevent accidental credential exposure:🔒 Suggested placeholder format
persistence: type: postgres - # Demo value only — see the note above; do not commit real credentials. - url: postgres://queryflux:queryflux@postgres.queryflux.svc.cluster.local:5432/queryflux + # Replace USERNAME, PASSWORD, and HOST before deploying. + # For production, use config.existingSecret instead (see note above). + url: postgres://USERNAME:PASSWORD@postgres.queryflux.svc.cluster.local:5432/queryflux🤖 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 `@charts/queryflux/examples/production-values.yaml` around lines 28 - 31, Replace the literal Postgres connection string under the persistence.url key with a clear placeholder (e.g., a named environment variable or templated value) so the production-values example contains no real credentials; update the persistence.type comment if needed to indicate it's a placeholder and ensure any documentation or templating (e.g., values templating that reads persistence.url) expects the placeholder name instead of a hard-coded URL.charts/queryflux/templates/service.yaml (1)
16-17: ⚡ Quick winClarify Service
targetPort: it’s using the port name, while Deployment uses numerictargetPort
charts/queryflux/templates/service.yamlsetstargetPort: {{ $port.name }}(so Service routes by named port).charts/queryflux/templates/deployment.yamldefines- name: {{ $port.name }}andcontainerPort: {{ $port.targetPort }}, so the named-port wiring is consistent and the routing works.- Remaining concern is clarity:
service.ports.targetPortis used by the Deployment forcontainerPort, not by the Service, so the value name can be misleading for users.♻️ Suggested fix
Option A: Use the numeric
targetPortin the Service (keeping the namedport/namefield for Service + Ingress):- port: {{ $port.port }} - targetPort: {{ $port.name }} + targetPort: {{ $port.targetPort }} protocol: {{ $port.protocol }} name: {{ $port.name }}🤖 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 `@charts/queryflux/templates/service.yaml` around lines 16 - 17, The Service currently sets targetPort to the port's name ({{ $port.name }}), which is confusing because the Deployment's containerPort is numeric ({{ $port.targetPort }}); change the Service to use the numeric targetPort ({{ $port.targetPort }}) while keeping the Service port/name mapping ({{ $port.port }} and {{ $port.name }}) so the Service forwards directly to the Deployment's containerPort; verify the Deployment still defines the container port with name {{ $port.name }} and containerPort {{ $port.targetPort }} for consistency.
🤖 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/workflows/ci.yml:
- Around line 47-53: Update the helm job to follow least-privilege: on the
actions/checkout step (the checkout action used in the helm job) set
persist-credentials: false to avoid leaking runner credentials, and add an
explicit job-level permissions block (e.g., permissions: contents: read) so the
job only has minimal read access; ensure these changes are applied within the
helm job definition that contains the checkout and helm steps.
In `@charts/queryflux/Chart.yaml`:
- Line 6: Replace the non-deterministic appVersion value by setting appVersion
to the concrete release tag (e.g., "0.1.0") instead of "latest" in Chart.yaml;
update this field on each release and ensure CI publishes the matching image tag
(ghcr.io/lakeops-org/queryflux:<that-version>) so chart installs are
reproducible and align with image.tag defaults.
In `@charts/queryflux/templates/hpa.yaml`:
- Around line 15-31: The HPA template currently renders an empty metrics array
when both .Values.autoscaling.targetCPUUtilizationPercentage and
.Values.autoscaling.targetMemoryUtilizationPercentage are unset, causing
Kubernetes validation to fail; fix it by adding a guard around the metrics block
(or around the whole HPA resource) that only renders the HPA/metrics when at
least one of .Values.autoscaling.targetCPUUtilizationPercentage or
.Values.autoscaling.targetMemoryUtilizationPercentage is set (e.g., use a
conditional like if or .Values.autoscaling.targetCPUUtilizationPercentage
.Values.autoscaling.targetMemoryUtilizationPercentage), or alternatively emit a
sensible default single metric (e.g., CPU) when none are provided; update the
template section that contains the metrics: block to implement this check so the
final rendered HPA always contains at least one metric.
In `@charts/queryflux/templates/tests/test-connection.yaml`:
- Line 15: The test YAML currently pins the container image as "image:
busybox:1.36" which is outdated and has known CVEs; update the image key to a
patched BusyBox tag (for example "busybox:1.36.1-r2" or a newer 1.38.x tag) or
pin to a known-safe digest in the same "image" field in the test-connection.yaml
template to ensure the test uses a CVE-patched BusyBox.
In `@charts/queryflux/values.schema.json`:
- Around line 106-109: The schema and Service template are inconsistent:
values.schema.json defines targetPort as an integer but templates/service.yaml
uses {{ $port.name }}; either remove/rename targetPort from the schema to
document that services use named ports (update values.schema.json and any docs
for the ports list) or change the Service template to reference the integer
targetPort (replace {{ $port.name }} with the port's targetPort value lookup) so
the template consumes the integer defined in values.schema.json; locate the
ports loop in templates/service.yaml and the targetPort property in
values.schema.json to apply the corresponding change across both files.
---
Nitpick comments:
In `@charts/queryflux/examples/external-config-values.yaml`:
- Around line 19-26: The service.ports.studio block contains redundant fields
when studio.enabled is false; remove the nested keys name, port, targetPort and
protocol under service.ports.studio and keep only enabled: false to simplify the
values file and avoid confusion with unused configuration (look for the
service.ports.studio entry and the studio.enabled flag to make the change).
In `@charts/queryflux/examples/production-values.yaml`:
- Around line 28-31: Replace the literal Postgres connection string under the
persistence.url key with a clear placeholder (e.g., a named environment variable
or templated value) so the production-values example contains no real
credentials; update the persistence.type comment if needed to indicate it's a
placeholder and ensure any documentation or templating (e.g., values templating
that reads persistence.url) expects the placeholder name instead of a hard-coded
URL.
In `@charts/queryflux/templates/service.yaml`:
- Around line 16-17: The Service currently sets targetPort to the port's name
({{ $port.name }}), which is confusing because the Deployment's containerPort is
numeric ({{ $port.targetPort }}); change the Service to use the numeric
targetPort ({{ $port.targetPort }}) while keeping the Service port/name mapping
({{ $port.port }} and {{ $port.name }}) so the Service forwards directly to the
Deployment's containerPort; verify the Deployment still defines the container
port with name {{ $port.name }} and containerPort {{ $port.targetPort }} for
consistency.
In `@charts/queryflux/templates/servicemonitor.yaml`:
- Around line 25-26: Prometheus requires scrapeTimeout < interval, but the
template uses .Values.serviceMonitor.interval and
.Values.serviceMonitor.scrapeTimeout without validation; add a Helm template
pre-check that parses both values to numeric seconds (e.g., strip trailing "s"
with replace and convert with atoi) and call fail (Helm's fail function) if
scrapeTimeout >= interval so installation fails early; implement this check near
the top of the servicemonitor.yaml template referencing
.Values.serviceMonitor.interval and .Values.serviceMonitor.scrapeTimeout and
include a clear error message indicating the offending values.
In `@scripts/check-helm-chart.sh`:
- Around line 54-57: The boolean chain using "grep -q ... && grep -q ... ||
fail" is confusing and triggers shellcheck; replace it with explicit,
independent checks so each grep result is evaluated clearly and failure is
invoked only if either check misses: run grep -q for '{{
.Values.existingSecret.usernameKey }}' against
"$CHART_DIR/templates/secret.yaml" and separately for '{{
.Values.existingSecret.passwordKey }}', and call the existing fail function
(referenced as fail) if either check fails; this keeps CHART_DIR and
templates/secret.yaml references and makes the intent explicit and
shellcheck-compliant.
🪄 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
Run ID: 555b2363-9872-498f-8474-ac81be12f2c3
📒 Files selected for processing (23)
.github/workflows/ci.ymlMakefileREADME.mdcharts/queryflux/Chart.yamlcharts/queryflux/README.mdcharts/queryflux/examples/external-config-values.yamlcharts/queryflux/examples/production-values.yamlcharts/queryflux/templates/NOTES.txtcharts/queryflux/templates/_helpers.tplcharts/queryflux/templates/configmap.yamlcharts/queryflux/templates/deployment.yamlcharts/queryflux/templates/hpa.yamlcharts/queryflux/templates/ingress.yamlcharts/queryflux/templates/networkpolicy.yamlcharts/queryflux/templates/pdb.yamlcharts/queryflux/templates/secret.yamlcharts/queryflux/templates/service.yamlcharts/queryflux/templates/serviceaccount.yamlcharts/queryflux/templates/servicemonitor.yamlcharts/queryflux/templates/tests/test-connection.yamlcharts/queryflux/values.schema.jsoncharts/queryflux/values.yamlscripts/check-helm-chart.sh
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.
Adds a provider-neutral Helm chart for deploying QueryFlux on Kubernetes, plus CI validation.
Notable design points:
config.yamlverbatim and does not interpolate environment variables into it. Any secret in the config (e.g. a Postgres URL with a password) would otherwise sit in plaintext in a ConfigMap.config.existingSecretlets operators mount the whole config from a Secret instead; it takes precedence overexistingConfigMapandconfig.create.existingSecret. Generated password is preserved across upgrades (re-read from the existing Secret).scripts/check-helm-chart.sh, which checks required files and runshelm lint+helm templateagainst default values and every example. Wired into a helm job in.github/workflows/ci.yml.examples/production-values.yaml(ingress, TLS, HPA, PDB, ServiceMonitor, NetworkPolicy, topology spread) andexamples/external-config-values.yaml(externally managed config + server-only image).Testing
make helm-checkpasses locally (helm lint + helm template clean for default values and both examples).Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores