Skip to content

refactor(helm): Standardize template formatting across deployments:#1822

Merged
junhaoliao merged 11 commits intoy-scope:mainfrom
junhaoliao:helm-fix-format
Jan 5, 2026
Merged

refactor(helm): Standardize template formatting across deployments:#1822
junhaoliao merged 11 commits intoy-scope:mainfrom
junhaoliao:helm-fix-format

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Dec 19, 2025

- Add `| int` casts to prevent scientific notation in `configmap.yaml`.
- Use named ports for Web UI and Query Scheduler health checks.
- Quote dynamically-generated host values for consistent string formatting.
- Reorder `volumeMounts` to list template usages before static items.

Description

Note

This PR is part of the ongoing work for #1309. More PRs will be submitted until the Helm chart is complete and fully functional.

This PR improves template formatting and consistency in the Helm chart:

  • Add | int casts to all integer values in configmap.yaml to ensure proper YAML type handling; otherwise, integers values might be treated as floats, and scientific notation might be used.
  • Use named ports for Web UI and Query Scheduler health checks
  • Add quotes around dynamically-generated host values in configmap.yaml for consistent string formatting
  • Reorder volumeMounts in reducer-deployment.yaml and query-scheduler-deployment.yaml to follow the established ordering convention (template helper usages before static items)

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Inspected formatting of all existing templates and confirmed the styles are consistent, all integers are | int processed.
  2. Ran the test.sh script and observed the cluster was up:
junhao@ASUS-X870E:~/workspace/5-clp/tools/deployment/package-helm$ ./test.sh 
...
All jobs completed and services are ready.

Summary by CodeRabbit

  • Chores
    • Improved deployment config typing and quoting so numeric and host fields are parsed correctly.
    • Services now explicitly read the central config file at startup via added config argument.
    • Exposed worker concurrency and reducer upsert-interval settings for performance tuning.
    • Readiness and liveness probes updated to use named container ports across services.
    • Restored and reordered several volume mounts and service start commands for Redis and results cache.
    • Chart version bumped.

✏️ Tip: You can customize this high-level summary in your review settings.

@junhaoliao junhaoliao requested a review from a team as a code owner December 19, 2025 06:42
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Walkthrough

Helm chart templates updated: numeric ConfigMap fields coerced to integers and host interpolations quoted; several deployments/statefulsets had volumeMount ordering adjusted, some containers had CLI args (including --config /etc/clp-config.yaml) added; readiness/liveness probes switched to named ports; Chart version bumped.

Changes

Cohort / File(s) Summary
ConfigMap numeric coercion & host quoting
tools/deployment/package-helm/templates/configmap.yaml
Added `
Query-scheduler
tools/deployment/package-helm/templates/query-scheduler-deployment.yaml
Reordered config volumeMount to follow logs; readiness/liveness probes changed to use named port query-scheduler instead of numeric 7000.
Reducer
tools/deployment/package-helm/templates/reducer-deployment.yaml
Reordered config volumeMount; added CLI args: --config /etc/clp-config.yaml, --concurrency "{{ .Values.workerConcurrency }}", --upsert-interval "{{ .Values.clpConfig.reducer.upsert_interval }}".
Web UI
tools/deployment/package-helm/templates/webui-deployment.yaml
Moved/added streams volumeMount earlier in list; readinessProbe TCP port changed from numeric 4000 to named port webui.
Compression-scheduler
tools/deployment/package-helm/templates/compression-scheduler-deployment.yaml
Moved config volumeMount into container volumeMounts; added --config /etc/clp-config.yaml to container command.
Database
tools/deployment/package-helm/templates/database-statefulset.yaml
Reordered config volumeMount to appear after data and logs mounts; mount properties unchanged.
Garbage collector
tools/deployment/package-helm/templates/garbage-collector-deployment.yaml
Reintroduced and reordered config and tmp volumeMounts (after streams); added --config /etc/clp-config.yaml to container command.
Query worker
tools/deployment/package-helm/templates/query-worker-deployment.yaml
Added config volumeMount at /etc/clp-config.yaml (readOnly) into container volumeMounts.
Redis
tools/deployment/package-helm/templates/redis-statefulset.yaml
Reintroduced redis-server command using /etc/redis/redis.conf and --requirepass $(REDIS_PASSWORD); restored config volumeMount for /etc/redis/redis.conf (subPath redis.conf, readOnly).
Results cache (Mongo)
tools/deployment/package-helm/templates/results-cache-statefulset.yaml
Restored config volumeMount for /etc/mongo/mongod.conf and reintroduced mongod args to run with --config /etc/mongo/mongod.conf and --bind_ip 0.0.0.0; mounts/args moved to follow logs mount.
Chart metadata
tools/deployment/package-helm/Chart.yaml
Bumped chart version from 0.1.2-dev.11 to 0.1.2-dev.14.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately describes the main objective of the changeset: standardizing Helm template formatting across multiple deployment configurations.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment on lines 73 to 79
readinessProbe:
{{- include "clp.readinessProbeTimings" . | nindent 12 }}
tcpSocket: &query-scheduler-health-check
port: 7000
port: "query-scheduler"
livenessProbe:
{{- include "clp.livenessProbeTimings" . | nindent 12 }}
tcpSocket: *query-scheduler-health-check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move these below the command like they are in other deployments?

kirkrodrigues
kirkrodrigues previously approved these changes Jan 5, 2026
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current PR title is a bit vague. Maybe we can come up with a multi-line commit message?

@junhaoliao junhaoliao requested review from kirkrodrigues and removed request for kirkrodrigues January 5, 2026 19:53
@junhaoliao junhaoliao changed the title refactor(helm): Improve template formatting and consistency. refactor(helm): Standardize template formatting across deployments: Jan 5, 2026
@junhaoliao junhaoliao merged commit 742f409 into y-scope:main Jan 5, 2026
24 checks passed
@junhaoliao junhaoliao deleted the helm-fix-format branch January 5, 2026 20:57
davidlion pushed a commit to davidlion/clp that referenced this pull request Jan 17, 2026
…-scope#1822)

- Add `| int` casts to prevent scientific notation in `configmap.yaml`.
- Use named ports for Web UI and Query Scheduler health checks.
- Quote dynamically-generated host values for consistent string formatting.
- Reorder `volumeMounts` to list template usages before static items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants