feat(deployment): Set up initial Helm chart for CLP package:#1614
Conversation
- Add database StatefulSet and db-table-creator Job. - Introduce `lint:helm` task to lint the chart using chart-testing (ct). - Update CI workflow to include Helm chart linting.
WalkthroughThis pull request adds Helm chart support to the CLP project, including CI/CD linting integration, Helm toolchain setup, and a complete Helm chart package for deployment. Changes span workflow configuration, task files, package dependencies, and new chart templates with manifests for database resources and configuration management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
| @@ -0,0 +1,38 @@ | |||
| # yamllint disable rule:document-start | |||
There was a problem hiding this comment.
this is the only modified line
There was a problem hiding this comment.
the file was extracted from chart-testing_3.14.0_linux_amd64.tar.gz
| yamllint>=1.35.1 | ||
| yamale>=6.0.0 No newline at end of file |
There was a problem hiding this comment.
chart-testing depends on not only yamllint but also yamale
| - "venv" | ||
| cmds: | ||
| - |- | ||
| . "{{.G_LINT_VENV_DIR}}/bin/activate" |
There was a problem hiding this comment.
to access yamllint and yamale
| docs \ | ||
| taskfile.yaml \ | ||
| taskfiles \ | ||
| tools/deployment |
There was a problem hiding this comment.
to exclude tools/deployment/package-helm
| # it doesn't exist. | ||
| # NOTE: This directory must not overlap with any path used in CLP's execution container. An error | ||
| # will be raised if so. | ||
| data_directory: "/tmp/clp/var/data" |
There was a problem hiding this comment.
note only absolute paths are accepted now
| shell: "bash" | ||
| run: "task lint:check-yaml" | ||
|
|
||
| - name: "Lint Helm charts" |
There was a problem hiding this comment.
we could just use https://github.com/helm/chart-testing-action but it's not doing anything fancier than installing yamllint, yamale (and also kind which we don't need in any GH workflow). if needed in the future, we can also set up a kind cluster in our taskfiles.
| {{/* | ||
| Creates timings for readiness probes (faster checks for quicker startup). | ||
| */}} | ||
| {{- define "clp.readinessProbeTimings" -}} | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 1 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 10 | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Creates timings for liveness probes. | ||
| */}} | ||
| {{- define "clp.livenessProbeTimings" -}} | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 3 | ||
| {{- end }} |
There was a problem hiding this comment.
note the numbers are different from the docker-compose healthcheck configurations. feel free to disagree
There was a problem hiding this comment.
Why do we want to change them? Is Kubernetes faster or something?
There was a problem hiding this comment.
in fact, init delay seems to be longer in k8s. i've tuned the values a bit
(i think we should also extend the timeout period in docker-compose to 4s, for avoiding false negatives on hosts with higher loads. if you agree, we can submit an issue and address separately
the k8s healthcheck mechanism is easier to be read so (as a writer) i feel inline comments are unnecessary. let me know if anything needs to be clarified and put into inline comments.
| home: "https://github.com/y-scope/clp" | ||
| icon: "https://github.com/y-scope/clp/raw/main/docs/src/clp-logo.png" | ||
| sources: ["https://github.com/y-scope/clp"] | ||
| maintainers: [{name: "y-scope", email: "dev@yscope.com"}] |
There was a problem hiding this comment.
the name needs to be a valid github org / user name
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/clp-lint.yaml(2 hunks)chart_schema.yaml(1 hunks)components/clp-package-utils/clp_package_utils/general.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/package-template/src/etc/credentials.template.yml(1 hunks)ct.yaml(1 hunks)lint-requirements.txt(1 hunks)taskfiles/lint.yaml(4 hunks)taskfiles/toolchains.yaml(2 hunks)tools/deployment/package-helm/.helmignore(1 hunks)tools/deployment/package-helm/Chart.yaml(1 hunks)tools/deployment/package-helm/templates/_helpers.tpl(1 hunks)tools/deployment/package-helm/templates/configmap.yaml(1 hunks)tools/deployment/package-helm/templates/database-data-pv.yaml(1 hunks)tools/deployment/package-helm/templates/database-logs-pv.yaml(1 hunks)tools/deployment/package-helm/templates/database-secret.yaml(1 hunks)tools/deployment/package-helm/templates/database-service.yaml(1 hunks)tools/deployment/package-helm/templates/database-statefulset.yaml(1 hunks)tools/deployment/package-helm/templates/db-table-creator-job.yaml(1 hunks)tools/deployment/package-helm/test.sh(1 hunks)tools/deployment/package-helm/values.yaml(1 hunks)tools/deployment/presto-clp/scripts/init.py(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
📚 Learning: 2025-08-16T10:24:29.316Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/pyproject.toml:11-13
Timestamp: 2025-08-16T10:24:29.316Z
Learning: In the CLP project's integration-tests directory, the integration tests package is not intended to be shipped or distributed - only the tests are leveraged for local testing purposes, so console script entry points should not be included in pyproject.toml.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
.github/workflows/clp-lint.yamltools/deployment/package-helm/templates/_helpers.tpltaskfiles/lint.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
.github/workflows/clp-lint.yamltaskfiles/lint.yaml
📚 Learning: 2025-08-14T18:23:39.476Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1119
File: taskfile.yaml:429-433
Timestamp: 2025-08-14T18:23:39.476Z
Learning: In the webui workspace, the exclude patterns ".tmp" and ".vite-temp" are Vite-specific temporary directories that only apply to the client package. The common and server packages use TypeScript compiler (tsc) directly and don't generate these Vite temporary files, so these exclude patterns should not be applied to their node_modules checksum computations.
Applied to files:
tools/deployment/package-helm/.helmignore
📚 Learning: 2025-08-20T05:38:30.720Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: taskfile.yaml:545-547
Timestamp: 2025-08-20T05:38:30.720Z
Learning: The python component directories in the y-scope/clp repository are kept very clean and only contain essential files: source code directories, pyproject.toml, poetry.lock, README.md, and the dist/ directory. No cache directories, virtual environments, or other transient files exist in the component directories, so additional exclude patterns beyond "dist/**/*" are unnecessary.
Applied to files:
tools/deployment/package-helm/.helmignore
📚 Learning: 2025-08-17T15:26:22.790Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:0-0
Timestamp: 2025-08-17T15:26:22.790Z
Learning: In the CLP project, Yoda-style conditions (literal on the left) are only used for `== 0` checks, not for other comparison operators like `>` or `<`. This is specifically to catch accidental assignment operators (writing `=` instead of `==`) in equality comparisons against zero.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-07-08T17:20:02.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-10-27T04:05:01.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1507
File: taskfiles/deps/lock.yaml:66-76
Timestamp: 2025-10-27T04:05:01.560Z
Learning: In the CLP project, the `uv` CLI tool is documented as a prerequisite in docs/src/dev-docs/building-package.md and is not installed via a task dependency like the rust toolchain. UV is expected to be available in the environment rather than being provisioned by the taskfile system.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Applied to files:
components/clp-py-utils/clp_py_utils/clp_config.pytools/deployment/presto-clp/scripts/init.py
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
tools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-07-25T21:29:48.947Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1126
File: .gitignore:5-5
Timestamp: 2025-07-25T21:29:48.947Z
Learning: In the CLP project, the .clang-format file is maintained in the yscope-dev-utils submodule and copied over to the main CLP repository, so it should be ignored in .gitignore to prevent accidental commits of the copied file and maintain the single source of truth in the submodule.
Applied to files:
tools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-07-05T03:38:16.779Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
tools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.
Applied to files:
tools/deployment/presto-clp/scripts/init.pycomponents/package-template/src/etc/credentials.template.yml
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-08-24T17:10:44.565Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: taskfiles/lint.yaml:780-781
Timestamp: 2025-08-24T17:10:44.565Z
Learning: The CLP repository is gradually migrating from Black to Ruff for Python formatting. Some modules haven't been set up for Ruff yet, so there's intentional mixed usage of Black and Ruff formatters during this transition period. This should not be flagged as an inconsistency in PR reviews.
Applied to files:
lint-requirements.txt
🧬 Code graph analysis (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
get_config_value(28-42)
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/database-data-pv.yaml
[error] 8-8: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
tools/deployment/package-helm/templates/configmap.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
tools/deployment/package-helm/templates/database-service.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 9-9: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/database-secret.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 10-10: too many spaces inside braces
(braces)
[error] 10-10: too many spaces inside braces
(braces)
[error] 11-11: too many spaces inside braces
(braces)
[error] 11-11: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/database-logs-pv.yaml
[error] 8-8: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
tools/deployment/package-helm/templates/db-table-creator-job.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 30-30: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 38-38: duplication of key "name" in mapping
(key-duplicates)
[error] 41-41: too many spaces inside braces
(braces)
[error] 41-41: too many spaces inside braces
(braces)
[error] 43-43: duplication of key "name" in mapping
(key-duplicates)
[error] 46-46: duplication of key "name" in mapping
(key-duplicates)
[error] 54-54: too many spaces inside braces
(braces)
[error] 54-54: too many spaces inside braces
(braces)
[error] 57-57: duplication of key "name" in mapping
(key-duplicates)
[error] 59-59: too many spaces inside braces
(braces)
[error] 59-59: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/database-statefulset.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 9-9: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 50-50: too many spaces inside braces
(braces)
[error] 50-50: too many spaces inside braces
(braces)
[error] 65-65: too many spaces inside braces
(braces)
[error] 73-73: too many spaces inside braces
(braces)
[error] 79-79: too many spaces inside braces
(braces)
[error] 79-79: too many spaces inside braces
(braces)
[error] 90-90: too many spaces inside braces
(braces)
[error] 102-102: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: rust-checks
🔇 Additional comments (22)
lint-requirements.txt (1)
6-12: LGTM!The addition of yamale and reorganization of dependencies under clear section headers improves maintainability. The yamale dependency is necessary for chart-testing as noted in previous comments.
tools/deployment/package-helm/.helmignore (1)
1-23: LGTM!Standard Helm ignore patterns appropriately exclude version control, temporary files, and IDE artifacts from chart packaging.
.github/workflows/clp-lint.yaml (2)
28-28: LGTM!Setting fetch-depth to 0 is necessary for Helm chart linting with chart-testing, which needs access to the origin/main branch for comparison, as explained in the past review comments.
83-85: LGTM!The Helm chart linting step appropriately integrates chart validation into the CI workflow, placed logically after YAML linting.
components/clp-package-utils/clp_package_utils/general.py (1)
454-463: LGTM!The credential generation correctly aligns with the updated schema, using
usernamefor database and queue components while maintaining Redis with only a password field.ct.yaml (1)
1-8: LGTM!The chart testing configuration appropriately enables schema validation, maintainer validation, and YAML linting. The
check-version-increment: truesetting on Line 4 will enforce version bumps in subsequent PRs, which is good practice for chart releases.components/package-template/src/etc/credentials.template.yml (1)
1-13: All credential consumers have been successfully updated to use "username".The verification confirms the schema migration is complete and consistent across the codebase:
- All configuration loading uses
.username:database.usernameandqueue.usernameare read consistently in clp_config.py- Credential template correctly generates with
"username"key in general.py- All credential consumers (compress.py, archive_manager.py, controller.py, search.py, decompress.py, etc.) access credentials via
.database.usernameand.queue.username- The one occurrence of
"user"in clp_config.py line 193 is correct—it's the output mapping for the MySQL driver connection dictionary, which expects the"user"parameter by conventionNo remaining references to the old credential field access pattern were found.
taskfiles/toolchains.yaml (2)
43-46: Mise à jour : Helm 4 est maintenant stable, corrigez votre analyse.Le script get-helm-4 est la méthode d'installation officielle pour Helm 4, et l'exécution via curl | bash est une approche documentée. Helm v4.0.0 a été publié et n'est plus en version alpha/beta. Les drapeaux de sécurité
--proto "=https"et--tlsv1.2sont appropriés, et le drapeau --no-sudo est une option acceptée par le script d'installation.Votre concern concernant Helm 4 étant « actuellement en phase alpha/bêta » est incorrect — la version 4.0.0 est maintenant la version stable officielle avec des correctifs et des versions mineures prévues.
Likely an incorrect or invalid review comment.
32-41: Verify error message clarity for unsupported platforms through manual testing.The concern is reasonable: passing
FILE_SHA256: "unsupported"to theremote:download-and-extract-tartask will cause the toolchain installation to fail on unsupported platforms. However, I cannot verify whether the resulting error message is clear without access to the external yscope-dev-utils package implementation.To confirm whether this needs addressing, manually run the helm toolchain task on an unsupported platform (e.g., Linux ARM64) and verify whether the error message clearly indicates that Chart-testing is only supported on linux/amd64 and darwin/arm64, or if it produces a cryptic error about an invalid SHA256 hash or malformed URL.
components/clp-py-utils/clp_py_utils/clp_config.py (2)
363-364: LGTM: Queue credential key renamed to "username".The queue credentials now use the "username" key, consistent with the Queue class field naming (line 352) and the database credentials change.
235-236: Credential schema refactor verified across all templates.Verification confirms the credential key change from "user" to "username" has been applied consistently across the codebase:
components/package-template/src/etc/credentials.template.yml(lines 3, 8): Updated to use "username"tools/deployment/package-helm/values.yaml(line 46): Uses "username"components/package-template/src/etc/clp-config.yml: Template examples show "username"No remaining references to
database.userorqueue.userin credential contexts. The changes align with the Database and Queue class field naming and are correctly implemented.tools/deployment/package-helm/templates/configmap.yaml (2)
8-21: LGTM: CLP configuration properly templated.The clp-config.yml uses appropriate hardcoded container paths (
/var/data,/var/log) and correctly templates dynamic values like database name, type, query_engine, and storage_engine from values.yaml. The database host reference uses the Helm fullname helper to ensure correct service discovery within the cluster.
23-33: LGTM: MySQL logging configuration is properly configured.The mysql-logging.cnf enables general logging and error logging with appropriate file paths that align with the database logs PersistentVolume mount path.
tools/deployment/package-helm/Chart.yaml (1)
1-17: LGTM: Chart metadata is well-formed.The Helm chart metadata is properly structured with appropriate versioning, maintainer information, and descriptive keywords. The appVersion "0.6.1-dev" correctly indicates this is a development release.
tools/deployment/package-helm/templates/database-secret.yaml (1)
10-11: Critical: Password references incorrect value path.Line 11 references
.Values.passwordinstead of.Values.credentials.database.password, which is inconsistent with the username reference on line 10 and likely incorrect.Apply this diff to fix the password reference:
stringData: username: {{ .Values.credentials.database.username | quote }} - password: {{ .Values.password | quote }} + password: {{ .Values.credentials.database.password | quote }}Likely an incorrect or invalid review comment.
tools/deployment/package-helm/templates/database-service.yaml (1)
9-22: The review comment is incorrect; no port range issue exists.The configuration in
values.yamlsetsclpConfig.database.port: 30306, which is within the valid Kubernetes NodePort range (30000-32767). The DEFAULT_PORT of 3306 inclp_config.pyis the internal database port, not the external NodePort value used in the Helm template. The service template correctly uses the configured external port (30306) for the NodePort assignment, so no validation or configuration changes are required.Likely an incorrect or invalid review comment.
tools/deployment/package-helm/values.yaml (1)
29-29: Verify the intended use of port 30306.Port 30306 falls within the Kubernetes NodePort range (30000-32767). For an internal database service, the standard MySQL/MariaDB port 3306 is more typical unless external access via NodePort is intentionally required. Please confirm this configuration aligns with the deployment architecture.
tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
1-59: LGTM: Well-structured database initialization Job.The Job appropriately uses an initContainer to wait for database availability before running the table creation logic. The environment variable sourcing from Secrets and ConfigMap mounting follow Kubernetes best practices.
taskfiles/lint.yaml (2)
59-70: LGTM: Proper integration of Helm chart linting.The new Helm linting task correctly depends on the Helm toolchain and virtual environment, and executes
ct lintfor chart validation. The aliases forcheck-helmandfix-helmmaintain consistency with other linting tasks.
116-128: Helm templates are correctly excluded — no action needed.Verification confirms that
tools/deployment/package-helmis a sibling directory totools/deployment/package, not a subdirectory. Since the yamllint configuration explicitly lists only specific paths (includingtools/deployment/packagebut nottools/deployment/package-helm), the Helm templates remain properly excluded. The directory structure and explicit path specification prevent yamllint from attempting to lint Go template syntax in the Helm templates.chart_schema.yaml (1)
1-38: LGTM: Standard Helm chart schema definition.This schema follows the standard Helm chart metadata structure and uses yamale syntax appropriately for chart-testing validation. The reusable object schemas for maintainer, dependency, and import-value align with Helm conventions.
tools/deployment/package-helm/templates/_helpers.tpl (1)
91-113: LGTM: Well-designed PersistentVolume helper template.The
clp.createLocalPvhelper appropriately uses local storage with node affinity and theRetainreclaim policy, which is suitable for local PersistentVolumes. The parameterization via dict allows flexible reuse across different PV definitions.
| {{- include "clp.createLocalPv" (dict | ||
| "root" . | ||
| "name" (printf "%s-database-data" (include "clp.fullname" .)) | ||
| "component" "database" | ||
| "capacity" "20Gi" | ||
| "accessModes" (list "ReadWriteOnce") | ||
| "hostPath" (printf "%s/database" .Values.clpConfig.data_directory) | ||
| ) }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider storage class alternatives for production deployments.
Similar to the database logs PV, this local hostPath PersistentVolume is suitable for development/testing but has the same production limitations (node affinity requirements, no portability, no replication).
Consider making the storage class configurable to allow production-grade persistent storage backends.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 8-8: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| {{- include "clp.createLocalPv" (dict | ||
| "root" . | ||
| "name" (printf "%s-database-logs" (include "clp.fullname" .)) | ||
| "component" "database" | ||
| "capacity" "5Gi" | ||
| "accessModes" (list "ReadWriteOnce") | ||
| "hostPath" (printf "%s/database" .Values.clpConfig.logs_directory) | ||
| ) }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider storage class alternatives for production deployments.
The local hostPath PersistentVolume is suitable for development/testing but has limitations for production:
- Requires node affinity to ensure the pod schedules on the same node as the hostPath
- Not portable across nodes
- No replication or backup capabilities
For production deployments, consider making the storage class configurable in values.yaml to allow users to specify network-attached storage (e.g., AWS EBS, GCE PD, NFS) instead of hostPath.
Example enhancement for values.yaml:
storage:
database:
logs:
storageClass: local-path # or dynamic provisioner
capacity: 5Gi🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 8-8: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/database-logs-pv.yaml around lines 1
to 8, the template currently creates a hostPath-based PersistentVolume which is
suitable only for dev/testing; make the storage configurable so production can
use a storageClass and dynamic provisioner. Modify values.yaml to add a
storage.database.logs block (storageClass, capacity, accessModes, and optional
hostPath) and update the template to check for
storage.database.logs.storageClass: if present render a PersistentVolumeClaim
(or a PV with storageClassName) using the provided storageClass and capacity; if
not provided fall back to the existing hostPath-based PV. Ensure defaults remain
backward-compatible and document the new values in values.yaml.
There was a problem hiding this comment.
I can be wrong but the rancher/local-path-provisioner that i have previously tried can't be specified in any Helm chart and therefore cannot be automated by helm install right? Then it should be more convenient to keep using local-path for development?
There was a problem hiding this comment.
@junhaoliao You're right that the rancher/local-path-provisioner itself needs to be installed separately as cluster infrastructure (it can't be automatically installed by your application's helm install). However, there's an important distinction to clarify:
Current approach in your PR:
- Manually creates hostPath-based PersistentVolumes in your Helm chart
- Requires pre-creating PVs before PVCs can bind
Using a StorageClass approach (e.g., "local-path"):
- The Local Path Provisioner allows you to create PVCs that are automatically provisioned on local directories of the nodes
- You only need to create PVCs with
storageClassName: local-pathin your chart - The provisioner automatically creates PVs when PVCs are created
- Actually simpler than manually creating PVs
For development workflows:
Kind (which you're using in your test.sh) doesn't include rancher/local-path-provisioner by default, but it does provide the standard StorageClass using the rancher.io/local-path provisioner in some configurations. Alternatively, users can install it once:
kubectl apply -f https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/local-path-storage.yamlRecommendation:
For a more flexible Helm chart, make the StorageClass configurable in values.yaml:
- Default to manual hostPath PVs for maximum compatibility (current approach)
- Allow users to optionally specify a
storageClassNameto use dynamic provisioning instead - Document that using
storageClassName: local-pathprovides a simpler experience if the provisioner is installed
This gives users the best of both worlds! Would you like me to show an example of how to make this configurable?
| {{- if eq .Values.clpConfig.database.type "mysql" }} | ||
| mysql:8.0.23 | ||
| {{- else }} | ||
| mariadb:10-jammy | ||
| {{- end }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Consider updating the MySQL image version.
The MySQL image is pinned to version 8.0.23 (released in 2021). Consider updating to a more recent 8.0.x or 8.4.x LTS release for security patches and improvements, or use a tag pattern like mysql:8.0 to automatically receive patch updates.
🌐 Web query:
What is the latest stable MySQL 8.0 version and are there known vulnerabilities in MySQL 8.0.23?
💡 Result:
Short answer
- Latest MySQL 8.0 stable (bug‑fix) release: MySQL 8.0.44 (GA / latest 8.0.x bug‑fix train as of Nov 17, 2025). [1][2]
- Known vulnerabilities in 8.0.23: yes — multiple CVEs affect 8.0.23 and earlier (for example CVE‑2021‑2308, CVE‑2021‑2166, CVE‑2021‑2208 and others). These were fixed in later 8.0.x releases; you should upgrade to a current 8.0.x (or newer) release and apply vendor patches. [3][4][5]
Sources
- Oracle MySQL server versions / release table (lists 8.0.43/8.0.44 in the 8.0 bug‑fix track). [turn0search3]
- NDB/Docs referencing NDB Cluster info up to 8.0.44. [turn1search5]
- CVE record / aggregators showing CVE‑2021‑2308 affects 8.0.23 and earlier. [turn2search1]
- NVD entry (example CVE‑2021‑2166) and other CVEs that list “8.0.23 and prior” as affected. [turn2search6]
- Vulnerability databases/notes (Rapid7 / vendor advisories) summarizing multiple CVEs impacting 8.0.23. [turn2search0]
If you want, I can list the specific CVE IDs that affect 8.0.23 and the releases where they were fixed.
Update MySQL image version from 8.0.23 to a current release.
MySQL 8.0.23 is affected by multiple known CVEs (including CVE-2021-2308, CVE-2021-2166, and CVE-2021-2208), which have been fixed in later 8.0.x releases. The latest stable MySQL 8.0 release is 8.0.44. Update to mysql:8.0.44 or use mysql:8.0 to receive automatic patch updates for improved security and stability.
🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/database-statefulset.yaml around
lines 28 to 32, the MySQL image tag is pinned to the vulnerable version 8.0.23;
update the image to a current, patched release by replacing mysql:8.0.23 with
either mysql:8.0.44 (explicit patched version) or mysql:8.0 (to track minor
security patches), and ensure any chart documentation or values references are
updated accordingly to reflect the new default image tag.
| {{- else }} | ||
| mariadb:10-jammy | ||
| {{- end }} | ||
| imagePullPolicy: "Always" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reconsider imagePullPolicy for third-party images.
Using imagePullPolicy: "Always" for stable third-party images (MySQL/MariaDB) may cause unnecessary registry pulls and slower pod startup. Consider using "IfNotPresent" for pinned version tags, or align with the pullPolicy from values.
- imagePullPolicy: "Always"
+ imagePullPolicy: {{ .Values.image.pullPolicy | default "IfNotPresent" | quote }}🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/database-statefulset.yaml around line
33, the container imagePullPolicy is hardcoded to "Always" which forces pulls
for stable third-party images; change it to use a safer default such as
"IfNotPresent" when image tags are pinned or make it configurable by reading the
pullPolicy from values.yaml (e.g., values.database.image.pullPolicy) so charts
can control the behavior, and update values.yaml and README accordingly.
| # it doesn't exist. | ||
| # NOTE: This directory must not overlap with any path used in CLP's execution container. An error | ||
| # will be raised if so. | ||
| data_directory: "/tmp/clp/var/data" |
There was a problem hiding this comment.
🧩 Analysis chain
Reconsider using /tmp for persistent data storage.
The /tmp directory is typically cleared on system reboot and not intended for persistent storage. Since the comment indicates this is where "other data (besides archives) are stored," consider using a more appropriate default path like /var/lib/clp/data or documenting that users should override this with a persistent volume mount.
Based on learnings (past comment notes "only absolute paths are accepted"), verify whether there are constraints requiring /tmp or if this is simply a development default.
I'll help verify this review comment by examining the Helm values configuration and how the data_directory is used throughout the deployment. Let me gather the necessary information.
🏁 Script executed:
#!/bin/bash
# Examine the values.yaml file and surrounding context
echo "=== Viewing values.yaml context ==="
cat tools/deployment/package-helm/values.yaml | head -50
echo -e "\n=== Searching for data_directory references in helm templates ==="
fd -e yaml -e yml tools/deployment/package-helm/templates/ --exec grep -l "data_directory" {} \;
echo -e "\n=== Searching for volumeMounts and persistent storage config ==="
grep -r "volumeMounts\|volumes\|PersistentVolume\|storage:" tools/deployment/package-helm/templates/ -A 2 -B 2 | head -100Length of output: 6647
🏁 Script executed:
#!/bin/bash
# Check how data_directory is used in templates
echo "=== Grep for data_directory usage patterns ==="
grep -r "data_directory" tools/deployment/package-helm/ -B 3 -A 3
echo -e "\n=== Check for any documentation about storage ==="
find tools/deployment/package-helm/ -name "README*" -o -name "*.md" | xargs cat 2>/dev/null | head -100Length of output: 1777
🏁 Script executed:
#!/bin/bash
# Look for deployment manifests and storage configuration
echo "=== Searching for Deployment manifests ==="
fd -e yaml -e yml tools/deployment/package-helm/templates/ | xargs ls -la
echo -e "\n=== Check for storage class or persistent volume references ==="
grep -r "storageClass\|persistentVolume\|claimName\|emptyDir" tools/deployment/package-helm/ -nLength of output: 2695
Reconsider using /tmp for persistent data storage.
/tmp should be used for smaller, size-bounded files only; data that shall survive a boot cycle shouldn't be placed in /tmp/. The /tmp directory is cleaned up after 10 days by default, and cleaned when it's either mounted or /usr is mounted, which regularly happens on boot.
Since this Helm chart configures persistent data via hostPath PersistentVolumes backed by /tmp/clp/var/data, the data is at risk of loss on node reboot or during automatic cleanup cycles. Consider changing the default to a persistent directory like /var/lib/clp/data or /opt/clp/data, and document that users must override this with a proper persistent volume mount for production deployments.
🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around line 36, the default
data_directory is set to /tmp/clp/var/data which is unsafe for persistent
storage; change the default to a persistent path such as /var/lib/clp/data or
/opt/clp/data, update any related hostPath PV defaults to match, and add a short
comment/instructions in the values file noting that users should override this
with a proper persistent volume mount for production deployments.
| # Location where logs are stored. It will be created if it doesn't exist. | ||
| # NOTE: This directory must not overlap with any path used in CLP's execution container. An error | ||
| # will be raised if so. | ||
| logs_directory: "/tmp/clp/var/log" |
There was a problem hiding this comment.
Reconsider using /tmp for log storage.
Similar to the data directory concern, /tmp/clp/var/log may not persist across reboots. Consider a default like /var/log/clp or document that users should override this for production deployments.
🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around line 41, the default
logs_directory is set to /tmp/clp/var/log which is ephemeral; change the default
to a persistent path such as /var/log/clp (or make it configurable with a clear
comment), and update documentation/README to instruct users to override the
value for production deployments if they need a different path or use mounted
persistent storage.
| credentials: | ||
| # Database credentials | ||
| database: | ||
| username: "clp-user" | ||
| password: "pass" |
There was a problem hiding this comment.
Replace hardcoded credentials with placeholder values and document secure credential management.
The hardcoded password "pass" poses a security risk. For a production-ready Helm chart, use placeholder values and document in comments that users must override these with secure credentials before deployment.
Apply this diff:
credentials:
# Database credentials
+ # SECURITY: Override these values with secure credentials before deploying to production.
+ # Consider using Kubernetes Secrets, external secret management (e.g., HashiCorp Vault),
+ # or helm install --set flags to provide credentials securely.
database:
username: "clp-user"
- password: "pass"
+ password: "CHANGEME"🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around lines 43 to 47, replace the
hardcoded database credentials (especially the password "pass") with
non-sensitive placeholder values and add a brief comment instructing users to
override them via --set, a separate secrets values file, or a Kubernetes Secret;
update the username to a placeholder like "<dbUsername>" and the password to
"<dbPassword>" and add a comment referencing secure credential management (e.g.,
use sealed-secrets/vault/K8s Secret) so deployers are required to supply real
secrets for production.
There was a problem hiding this comment.
is it possible to check whether a non-default / non-empty value has been provided during helm install?
There was a problem hiding this comment.
@junhaoliao Yes, absolutely! Helm provides several ways to validate that non-default values have been provided during helm install. Here are the main approaches:
1. Using fail function to reject default values
You can add validation in your templates (commonly in _helpers.tpl) that checks if the password is still the default and fails the installation:
{{- define "clp.validateCredentials" -}}
{{- if eq .Values.credentials.database.password "CHANGEME" }}
{{- fail "ERROR: Default database password detected. You must override credentials.database.password with a secure value using --set or a values file." }}
{{- end }}
{{- if eq .Values.credentials.database.password "pass" }}
{{- fail "ERROR: Default database password detected. You must override credentials.database.password with a secure value using --set or a values file." }}
{{- end }}
{{- end }}Then include this validation in one of your templates (e.g., at the top of database-secret.yaml):
{{- include "clp.validateCredentials" . }}
apiVersion: v1
kind: Secret
# ... rest of the secret definition2. Using required function for mandatory values
If you want to force users to always provide a value (not relying on a default at all), you can use the required function:
# In database-secret.yaml
stringData:
username: {{ required "A valid .Values.credentials.database.username is required!" .Values.credentials.database.username | quote }}
password: {{ required "A valid .Values.credentials.database.password is required!" .Values.credentials.database.password | quote }}However, this would fail even with the default value in values.yaml, so you'd need to remove the defaults entirely.
3. Checking for empty values
You can also check for empty strings:
{{- if or (empty .Values.credentials.database.password) (eq .Values.credentials.database.password "CHANGEME") }}
{{- fail "Database password must be set to a non-empty, non-default value" }}
{{- end }}Recommended approach
For your use case, I'd recommend option 1 (using fail with a validation helper) because it:
- Allows you to keep sensible defaults for testing/development
- Provides clear error messages to users who forget to override credentials
- Can be easily extended to validate multiple fields
- Keeps validation logic centralized
You could add this validation to _helpers.tpl and call it from your database-secret.yaml template to ensure it runs during installation.
| database_username = _get_required_config_value( | ||
| credentials, "database.username", credentials_file_path | ||
| ) | ||
| database_password = _get_required_config_value( | ||
| credentials, "database.password", credentials_file_path | ||
| ) | ||
| except KeyError: | ||
| return False | ||
| env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER"] = database_user | ||
| env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER"] = database_username | ||
| env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_PASSWORD"] = database_password |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Consider renaming environment variable for consistency.
The code now reads database.username from the credentials file (line 168) and stores it in the database_username variable (line 167), but the environment variable key on line 175 is still PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER.
For naming consistency with the credential schema change, consider updating the environment variable key:
- env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER"] = database_username
+ env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USERNAME"] = database_usernameNote: This change would also require updating the Presto coordinator configuration to read from the new environment variable name. Verify that the Presto coordinator side is updated accordingly.
Run the following script to check if the Presto coordinator references this environment variable:
🏁 Script executed:
#!/bin/bash
# Search for references to the database user environment variable in Presto config
rg -nP 'METADATA_DATABASE_(USER|USERNAME)' tools/deployment/presto-clp/Length of output: 292
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if clp.properties was modified in this PR and look for the full context
fd -n 'clp.properties' tools/deployment/presto-clp/ -exec cat {} \;
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for any other references to DATABASE_USER or DATABASE_USERNAME in the entire presto-clp directory
rg -n 'DATABASE_USER|database.user|database.username' tools/deployment/presto-clp/ -C 2Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Check if there are any migration notes or comments about the credential schema change
rg -n 'database\.user|database\.username' tools/deployment/presto-clp/ --type-list | head -20Length of output: 389
🏁 Script executed:
#!/bin/bash
# Get git history to see if both files were changed together
git log -n 10 --oneline --name-status -- tools/deployment/presto-clp/scripts/init.py tools/deployment/presto-clp/coordinator/config-template/clp.properties 2>/dev/null || echo "Git history not available"Length of output: 1395
🏁 Script executed:
#!/bin/bash
# Search for the context around where database.username was introduced in init.py
rg -n 'database\.' tools/deployment/presto-clp/scripts/init.py -B 2 -A 2Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Check for other credential mappings to see if there's a pattern of key vs env var name mismatches
rg -n '_get_required_config_value' tools/deployment/presto-clp/scripts/init.py -A 2 | head -40Length of output: 1265
Consider renaming environment variable for naming consistency across files.
The code now reads database.username from the credentials file (line 168), but the environment variable key on line 175 is still PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER (rather than USERNAME). This inconsistency also affects tools/deployment/presto-clp/coordinator/config-template/clp.properties (line 5), which reads this environment variable.
For complete naming consistency, both files would need coordinated updates:
# init.py line 175
- env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER"] = database_username
+ env_vars["PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USERNAME"] = database_username# clp.properties line 5
-clp.metadata-db-user=${PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USER}
+clp.metadata-db-user=${PRESTO_COORDINATOR_CLPPROPERTIES_METADATA_DATABASE_USERNAME}Committable suggestion skipped: line range outside the PR's diff.
kirkrodrigues
left a comment
There was a problem hiding this comment.
Some comments and questions.
| command: [ | ||
| "mysqladmin", "ping", | ||
| "--silent", | ||
| "-h", "127.0.0.1" |
There was a problem hiding this comment.
Do we need to specify the username and password like we did for Docker Compose?
There was a problem hiding this comment.
we should. previously i removed the arguments and found services were up without restarting and thought that was fine, but actually the pod was reporting bad health. i updated the command to include the credentials now
| image: "busybox:latest" | ||
| command: [ | ||
| "sh", "-c", | ||
| "until nc -z {{ include "clp.fullname" . }}-database 3306; do echo waiting for database; sleep 2; done" |
| storage_engine: "clp-s" | ||
| query_engine: "clp-s" |
There was a problem hiding this comment.
How would the choice between clp and clp-s work?
There was a problem hiding this comment.
sorry, not sure if i get this. mind clarifying the question?
There was a problem hiding this comment.
How are we going to have clp-json and clp-text packages in the Kubernetes flow?
There was a problem hiding this comment.
ah, users can just configure these two as both "clp-s" or both "clp"
There was a problem hiding this comment.
as discussed offline, there are mainly two ways to install the Helm chart:
- Users may pull our git repo directly, make modifications to
tools/deployment/package-helm/values.yamland runhelm install .to install the updated chart. - Users may also
helm install <release-name> <release-url> --set clpConfig.storage_engine="clp" --set clpConfig.query_engine="clp"orhelm install <release-name> <release-url> -f value-override.yaml, where the<release-url>can be a release tar or a HTTP static server (e.g., public s3, GitHub Pages).
| {{/* | ||
| Creates timings for readiness probes (faster checks for quicker startup). | ||
| */}} | ||
| {{- define "clp.readinessProbeTimings" -}} | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 1 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 10 | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Creates timings for liveness probes. | ||
| */}} | ||
| {{- define "clp.livenessProbeTimings" -}} | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 3 | ||
| {{- end }} |
There was a problem hiding this comment.
Why do we want to change them? Is Kubernetes faster or something?
| clp-config.yml: | | ||
| data_directory: "/var/data" | ||
| database: | ||
| auto_commit: false |
There was a problem hiding this comment.
Why do we need to set parameters like auto_commit that we don't usually set in clp-config.yaml?
There was a problem hiding this comment.
This was copied from the generated var/.clp-config.yaml. there's probably no difference but would it be better to keep it for explicitness? (easier diff as well
# Conflicts: # taskfiles/lint.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
35-35: Align imagePullPolicy with values configuration (duplicate comment).The
imagePullPolicy: "Always"remains hardcoded, which forces image pulls on every Job run. For tagged/versioned images, this is typically unnecessary and can slow down execution. This should use a configurable value from values.yaml for consistency with other resources in the chart.Apply this diff to make the pull policy configurable:
- imagePullPolicy: "Always" + imagePullPolicy: {{ .Values.image.clpPackage.pullPolicy | quote }}Ensure the corresponding default is set in
values.yaml:image: clpPackage: pullPolicy: "IfNotPresent"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/lint.yaml(4 hunks)tools/deployment/package-helm/templates/db-table-creator-job.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-11-17T22:58:50.056Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
Applied to files:
taskfiles/lint.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/db-table-creator-job.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks
🔇 Additional comments (4)
tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
1-63: YAMLlint errors are false positives; no action required.The static analysis tool reports "too many spaces inside braces" errors across multiple lines. These are false positives resulting from YAMLlint not properly understanding Helm template syntax (the
{{ }}notation is standard and required). These errors can be safely ignored or the file can be excluded from YAMLlint validation if configured at the project level.taskfiles/lint.yaml (3)
17-17: LGTM—helm tasks integrated correctly into check/fix flows.The placement after C++ checks and before other language checks follows the existing pattern.
Also applies to: 36-36
116-128: Verify whethertools/deployment/package-helm/should be excluded from yamllint.A past review comment references excluding
tools/deployment/package-helm, but the current yamllint configuration does not explicitly exclude this Helm chart directory. Since the Helm charts are linted separately viact lint, you should clarify:
- Should Helm template files be skipped by yamllint to avoid duplicate or conflicting lint rules?
- If so, add an exclusion pattern (e.g.,
-g '!tools/deployment/package-helm/**') to the yamllint command or document why files in this directory do not need yamllint validation.
59-70: Now I need to search for the actual definition ofG_HELM_TOOLCHAIN_ENV_FILEin the codebase to determine if it exists.Based on the verification results gathered, I can now generate the final rewritten review comment. The key findings are:
- ct's
--chart-dirsconfiguration specifies directories containing Helm charts and can be specified multiple times or separate values with commas- The
chart-dirssetting in ct.yaml tells ct where your Helm chart monorepo is- The script confirmed that
ct.yamlcontainschart-dirs: ["tools/deployment"]which will discover the chart attools/deployment/package-helm/- The Helm chart directory exists with proper structure (Chart.yaml, values.yaml, templates)
- Variables in the global scope should be prefixed with G_ to avoid conflicts with local variables - confirming the naming convention
The critical issue is that the first script's output was truncated (only showing 20 lines), so I cannot definitively confirm whether
G_HELM_TOOLCHAIN_ENV_FILEis actually exported by thetoolchains:helmtask. The review comment requests verification of this variable.Verify toolchain environment file configuration in Helm task setup.
The helm task depends on
{{.G_HELM_TOOLCHAIN_ENV_FILE}}, which must be defined as a global variable in the Taskfile. Variables in the global scope should be prefixed with G_ to avoid conflicts with local variables, and this variable needs to be exported by thetoolchains:helmdependency for thect lintcommand to execute properly.Additionally, the ct.yaml
chart-dirssetting tells ct where your Helm chart monorepo is, and the current configuration specifiestools/deploymentas the parent directory, which will automatically discover charts in subdirectories liketools/deployment/package-helm/.Confirm that:
G_HELM_TOOLCHAIN_ENV_FILEis defined in the globalvarssection oftaskfiles/toolchains.yamlor another included Taskfile- The helm task properly exports this variable for use in the lint task
| fsGroup: {{ .Values.securityContext.firstParty.gid }} | ||
| initContainers: | ||
| - name: "wait-for-database" | ||
| image: "busybox:latest" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider pinning the busybox image tag.
Using busybox:latest for the initContainer introduces potential reliability and reproducibility concerns. Pinning to a specific version ensures consistent behaviour across deployments.
Apply this diff to pin the image tag:
- image: "busybox:latest"
+ image: "busybox:1.36"Alternatively, make this configurable via values.yaml for greater flexibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image: "busybox:latest" | |
| image: "busybox:1.36" |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tools/deployment/package-helm/templates/database-statefulset.yaml (3)
27-33: Make database image configurable and avoid pinning to old MySQL 8.0.23.Right now the DB image (mysql:8.0.23 vs mariadb:10-jammy) is hard-coded here. That makes upgrading for security fixes or swapping to a distro variant require a template change rather than a values change, and 8.0.23 is a 2021-era MySQL build with multiple known CVEs fixed in later 8.0.x releases.
Consider:
- Introducing
.Values.database.image.repository/.Values.database.image.tag(and, if needed, a type-specific map) and wiringimage:through those, similar toclp.image.ref, and- Keeping the default tag at a currently supported bugfix/MySQL LTS release in values.yaml rather than in the template.
This keeps the chart flexible for operators and makes it easier to track supported DB versions.
33-33: RelaximagePullPolicyfor the third-party database image.
imagePullPolicy: "Always"can slow restarts and pound the registry for a pinned DB tag. For a stable third-party image, consider either:
IfNotPresent, or- Reading a pull policy from values (e.g.,
.Values.database.image.pullPolicywith a default of"IfNotPresent").This keeps behaviour configurable across environments.
35-51: Separate root and application user database passwords.
MYSQL_ROOT_PASSWORDandMYSQL_PASSWORDboth pull from the samepasswordkey in the Secret, which effectively gives the application user full root credentials. That’s a meaningful security regression vs. having a distinct, stronger root secret kept for break-glass operations only.Suggested direction:
- Extend
database-secret.yamland values.yaml to carry a separaterootPassword(or similar) key.- Point
MYSQL_ROOT_PASSWORDat that new key while keepingMYSQL_USER/MYSQL_PASSWORDon the existing username/password pair.This keeps least-privilege boundaries clearer without changing connection behaviour for the app.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
lint-requirements.txt(1 hunks)tools/deployment/package-helm/templates/_helpers.tpl(1 hunks)tools/deployment/package-helm/templates/database-statefulset.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
tools/deployment/package-helm/templates/_helpers.tpl
📚 Learning: 2025-08-24T17:10:44.565Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: taskfiles/lint.yaml:780-781
Timestamp: 2025-08-24T17:10:44.565Z
Learning: The CLP repository is gradually migrating from Black to Ruff for Python formatting. Some modules haven't been set up for Ruff yet, so there's intentional mixed usage of Black and Ruff formatters during this transition period. This should not be flagged as an inconsistency in PR reviews.
Applied to files:
lint-requirements.txt
📚 Learning: 2025-11-17T22:58:50.056Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
Applied to files:
lint-requirements.txt
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/database-statefulset.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 9-9: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 22-22: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 23-23: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 36-36: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 50-50: too many spaces inside braces
(braces)
[error] 50-50: too many spaces inside braces
(braces)
[error] 65-65: too many spaces inside braces
(braces)
[error] 77-77: too many spaces inside braces
(braces)
[error] 83-83: too many spaces inside braces
(braces)
[error] 83-83: too many spaces inside braces
(braces)
[error] 94-94: too many spaces inside braces
(braces)
[error] 106-106: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: rust-checks
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (3)
lint-requirements.txt (1)
1-12: Well-organized refactoring with correct dependencies for chart-testing support.The reorganization into clear sections (C++, Python, YAML) with alphabetically ordered tools within each section improves maintainability. The addition of
yamale>=6.0.0correctly addresses the chart-testing dependency, as noted in prior feedback. The relocation ofblack>=24.4.2under the Python section provides better logical cohesion.tools/deployment/package-helm/templates/database-statefulset.yaml (1)
8-20: StatefulSet wiring and labelling look consistent.Selector labels, pod template labels, and the
serviceNameall line up, which should keep the headless service and StatefulSet identity behaving as expected.tools/deployment/package-helm/templates/_helpers.tpl (1)
53-59: Image helper supports deterministic CLP package versions.
clp.image.refcleanly ties the CLP package image tag to an explicit override or the chart’sAppVersion, which is a solid way to keep deployments reproducible and aligned with the chart version while still allowing controlled overrides via values.
| {{/* | ||
| Creates timings for readiness probes (faster checks for quicker startup). | ||
| */}} | ||
| {{- define "clp.readinessProbeTimings" -}} | ||
| initialDelaySeconds: 6 | ||
| periodSeconds: 2 | ||
| timeoutSeconds: 2 | ||
| failureThreshold: 10 | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Creates timings for liveness probes. | ||
| */}} | ||
| {{- define "clp.livenessProbeTimings" -}} | ||
| initialDelaySeconds: 60 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 4 | ||
| failureThreshold: 3 | ||
| {{- end }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making probe timings configurable via values.
The readiness/liveness timing helpers centralise defaults nicely, but they’re fully hard-coded. If you expect clusters with very different performance profiles, exposing these as optional values (e.g., .Values.probes.readiness.* / .Values.probes.liveness.* with these numbers as defaults) would let operators tune without editing templates, while still keeping sane defaults here.
| {{/* | ||
| Creates a local PersistentVolume. | ||
| Parameters (dict): | ||
| root: Root template context | ||
| name: PV name (full) | ||
| component: Component label | ||
| capacity: Storage capacity | ||
| accessModes: Access modes (list) | ||
| hostPath: Absolute path on host | ||
| */}} | ||
| {{- define "clp.createLocalPv" -}} | ||
| apiVersion: "v1" | ||
| kind: "PersistentVolume" | ||
| metadata: | ||
| name: {{ .name }} | ||
| labels: | ||
| {{- include "clp.labels" .root | nindent 4 }} | ||
| app.kubernetes.io/component: {{ .component | quote }} | ||
| spec: | ||
| capacity: | ||
| storage: {{ .capacity }} | ||
| accessModes: {{ .accessModes }} | ||
| persistentVolumeReclaimPolicy: "Retain" | ||
| storageClassName: "local-storage" | ||
| local: | ||
| path: {{ .hostPath }} | ||
| nodeAffinity: | ||
| required: | ||
| nodeSelectorTerms: | ||
| - matchExpressions: | ||
| - key: "kubernetes.io/hostname" | ||
| operator: "Exists" | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Creates a PersistentVolumeClaim. | ||
| Parameters (dict): | ||
| root: Root template context | ||
| component: Component label (used in name, labels, and selectors) | ||
| capacity: Storage capacity | ||
| accessModes: Access modes (list) | ||
| */}} | ||
| {{- define "clp.createPvc" -}} | ||
| apiVersion: "v1" | ||
| kind: "PersistentVolumeClaim" | ||
| metadata: | ||
| name: {{ include "clp.fullname" .root }}-{{ .component }} | ||
| labels: | ||
| {{- include "clp.labels" .root | nindent 4 }} | ||
| app.kubernetes.io/component: {{ .component | quote }} | ||
| spec: | ||
| accessModes: {{ .accessModes }} | ||
| storageClassName: "local-storage" | ||
| selector: | ||
| matchLabels: | ||
| {{- include "clp.selectorLabels" .root | nindent 6 }} | ||
| app.kubernetes.io/component: {{ .component | quote }} | ||
| resources: | ||
| requests: | ||
| storage: {{ .capacity }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
PV/PVC helpers look correct; storage class and access modes could be more flexible.
The clp.createLocalPv / clp.createPvc helpers standardise naming and labelling, which should keep binding predictable across templates. Two minor points to consider:
storageClassNameis hard-coded to"local-storage"in both helpers; mirroring a value (e.g.,.Values.storage.local.className) would avoid duplication with the StatefulSet PVCs and make non-local backends easier in future.accessModesis rendered inline; if you truly expect a list, you may want to usetoYaml+nindentor, alternatively, document that callers should pass a single-line YAML list string (e.g.,["ReadWriteOnce"]) to avoid surprises.
Overall the abstraction is useful; these tweaks would make it more adaptable.
| volumes: | ||
| - name: "config" | ||
| configMap: | ||
| name: {{ include "clp.fullname" . }}-config | ||
| persistentVolumeClaimRetentionPolicy: | ||
| whenDeleted: "Delete" | ||
| volumeClaimTemplates: | ||
| - metadata: | ||
| name: "database-data" | ||
| spec: | ||
| accessModes: ["ReadWriteOnce"] | ||
| storageClassName: "local-storage" | ||
| selector: | ||
| matchLabels: | ||
| {{- include "clp.selectorLabels" . | nindent 12 }} | ||
| app.kubernetes.io/component: "database" | ||
| resources: | ||
| requests: | ||
| storage: "20Gi" | ||
| - metadata: | ||
| name: "database-logs" | ||
| spec: | ||
| accessModes: ["ReadWriteOnce"] | ||
| storageClassName: "local-storage" | ||
| selector: | ||
| matchLabels: | ||
| {{- include "clp.selectorLabels" . | nindent 12 }} | ||
| app.kubernetes.io/component: "database" | ||
| resources: | ||
| requests: | ||
| storage: "5Gi" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
PVC templates align with local PV usage, but storage class is hard-coded.
The two volumeClaimTemplates match the component labels and local-storage class used by the PV helpers, so binding should be deterministic for the packaged chart. The only caveat is that storageClassName is hard-coded; if you ever want to support non-local storage backends, you may want to drive this from values rather than inlining "local-storage" in multiple places.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 83-83: too many spaces inside braces
(braces)
[error] 83-83: too many spaces inside braces
(braces)
[error] 94-94: too many spaces inside braces
(braces)
[error] 106-106: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/database-statefulset.yaml around
lines 80 to 110, the storageClassName is hard-coded to "local-storage" in both
volumeClaimTemplates; change this to use a Helm chart value so deployers can
override the storage class (e.g. replace the literal with a template reference
to a values key like .Values.persistence.storageClass or .Values.storageClass
and provide a sensible default in values.yaml), update values.yaml with the new
key and default ("local-storage") and adjust any documentation so users know how
to override it.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/deployment/package-helm/Chart.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: rust-checks
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (3)
tools/deployment/package-helm/Chart.yaml (3)
6-6: Verify appVersion aligns with the actual CLP release version.Ensure that appVersion "0.6.1-dev" corresponds to the version of CLP being packaged in this deployment. Please confirm this version is correct for the deployment target.
8-8: Verify icon URL is accessible and correct.The icon URL references a GitHub raw content path. Please confirm this URL is accessible and that the logo is correctly served before merging.
10-10: Maintainers format looks good.The maintainers field follows the suggested format from the previous review, with proper structure for name, email, and URL.
| apiVersion: "v2" | ||
| name: "clp" | ||
| description: "A Helm chart for CLP (Compressed Log Processor) package deployment" | ||
| type: "application" | ||
| version: "0.1.0" | ||
| appVersion: "0.6.1-dev" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reorganise required fields to the top of the file for clarity.
Based on Kirk's previous suggestion in review comments, please reorganise the file to group required/common Helm fields (apiVersion, name, version, appVersion) at the top, before optional fields like description and type. This improves readability and follows Helm chart conventions.
Apply this diff to reorganise the fields:
apiVersion: "v2"
name: "clp"
+version: "0.1.0"
+appVersion: "0.6.1-dev"
description: "A Helm chart for CLP (Compressed Log Processor) package deployment"
type: "application"
-version: "0.1.0"
-appVersion: "0.6.1-dev"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| apiVersion: "v2" | |
| name: "clp" | |
| description: "A Helm chart for CLP (Compressed Log Processor) package deployment" | |
| type: "application" | |
| version: "0.1.0" | |
| appVersion: "0.6.1-dev" | |
| apiVersion: "v2" | |
| name: "clp" | |
| version: "0.1.0" | |
| appVersion: "0.6.1-dev" | |
| description: "A Helm chart for CLP (Compressed Log Processor) package deployment" | |
| type: "application" |
🤖 Prompt for AI Agents
In tools/deployment/package-helm/Chart.yaml around lines 1 to 6, the Helm chart
fields are ordered with optional fields before some required ones; reorder the
YAML so required/common Helm fields (apiVersion, name, version, appVersion)
appear first, followed by optional fields like description and type, preserving
existing values exactly—move apiVersion, name, version, appVersion to the top of
the file and place description and type after them.
kirkrodrigues
left a comment
There was a problem hiding this comment.
A review of most other things. Still one more review to go to finish a complete pass.
| app.kubernetes.io/component: "database" | ||
| spec: | ||
| serviceName: {{ include "clp.fullname" . }}-database | ||
| replicas: 1 |
There was a problem hiding this comment.
I think this defaults to 1, but I gu8ess you want to be explicit for clarity?
There was a problem hiding this comment.
yes. people usually set up StatefulSets / Deployments with multiple replicas
| name: {{ include "clp.fullname" . }}-config | ||
| persistentVolumeClaimRetentionPolicy: | ||
| whenDeleted: "Delete" | ||
| volumeClaimTemplates: |
There was a problem hiding this comment.
Forgive my ignorance, but why do the objects in this not have/need labels in metadata?
There was a problem hiding this comment.
there are different purposes of setting labels in K8s:
-
StatefulSet metadata labels: Labels on the StatefulSet resource itself
- Used by Helm for lifecycle management (upgrades, rollbacks)
- Used for querying:
kubectl get statefulsets -l app.kubernetes.io/name=clp
-
Pod template labels: Labels on the Pods that will be created
- REQUIRED - must match the StatefulSet's
selector.matchLabels - Without these, the StatefulSet can't manage its pods
- REQUIRED - must match the StatefulSet's
it wasn't necessary to add metadata labels to VolumeClaimTemplate metadata since:
- PVCs don't need labels for functional reasons (unlike pods)
- they're tightly coupled to the
StatefulSetalready
that said, adding labels would be help consistency/observability so we should add those consistently
There was a problem hiding this comment.
fwiw, labels from the set.Spec.Selector.MatchLabels do get copied into the volumeClaimTemplates created PVC:
https://github.com/kubernetes/kubernetes/blob/4bed36e03e7bd699b089d33da6f7d7c9ef9eb661/pkg/controller/statefulset/stateful_set_utils.go#L402
There was actually a bug in 2019 where custom labels get overridden by set.Spec.Selector.MatchLabels.
As explain by https://github.com/kubernetes/kubernetes/pull/74941/files#r263894283
This code is intentional to allow the user of the statefulset to find the PVCs it creates.
I think it's fine to not specify any custom label for the volumeClaimTemplates metadata then.
there is no doc regarding the behaviour as the k8s maintainers admit:
On review history, there was not comment related to this code. And the above explanation of this method(line 141-143) just mentions the name of claim, not the label.
I'm still not sure this code is intentional or not.. I hope the specialists can help this.
| @@ -0,0 +1,8 @@ | |||
| {{- include "clp.createLocalPv" (dict | |||
| "root" . | |||
There was a problem hiding this comment.
I asked AI about this and got a reasonable answer, but do you know where I can find the docs for what . does in this context?
There was a problem hiding this comment.
the . represents the context of the template expansion
| nodeSelectorTerms: | ||
| - matchExpressions: | ||
| - key: "kubernetes.io/hostname" | ||
| operator: "Exists" |
There was a problem hiding this comment.
Can you explain this to me?
There was a problem hiding this comment.
this was to get over the restriction but i agree this is too permissive. i think it's more proper to use https://kubernetes.io/docs/reference/labels-annotations-taints/#node-role-kubernetes-io-control-plane instead. e.g.,
nodeAffinity:
required:
nodeSelectorTerms:
- matchExpressions:
- key: "node-role.kubernetes.io/control-plane"
operator: "Exists"let me refactor this template function so other node-role types can be specified as well
| fullnameOverride: "" | ||
|
|
||
| # Whether to allow sbin script accesses to services on the node host. | ||
| allowSbinScripts: true |
There was a problem hiding this comment.
This isn't a great name. How about allowHostAccessForSbinScripts?
| image: | ||
| clpPackage: | ||
| repository: "ghcr.io/y-scope/clp/clp-package" | ||
| pullPolicy: "IfNotPresent" |
There was a problem hiding this comment.
Do we need this? I don't think we're using it currently, right?
There was a problem hiding this comment.
we should use it in db-table-creator-job.yaml , and the default should be updated as "Always".
when users want to debug a local image, they can import the image into the cluster, --set the repository as the project name, tag as the image tag, and pullPolicy as IfNotPresent
| database: | ||
| type: "mariadb" # "mariadb" or "mysql" | ||
| host: "localhost" | ||
| port: 30306 |
There was a problem hiding this comment.
https://kubernetes.io/docs/reference/networking/ports-and-protocols/#node
only ports in range 30000-32767 is allowed to be mapped. i removed the thousand-bit from the well-known 3306 port and add that to 30000
…lane nodes. Update helper template to include `nodeRole` field with fallback logic.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
…rer intent in Helm values and templates.
…ply suggestions from code review Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
tools/deployment/package-helm/values.yaml (1)
43-47: Hardcoded credentials remain a security concern.The hardcoded password "pass" continues to pose a security risk. As discussed in previous reviews, this should be replaced with a placeholder and validated during deployment.
tools/deployment/package-helm/templates/db-table-creator-job.yaml (2)
21-31: initContainer wait logic is correct.The initContainer properly waits for the database service to be ready before starting the main container. The use of netcat to check port availability is a common and effective pattern.
Note: The use of
busybox:latesthas been previously flagged and should be pinned to a specific version.
32-63: LGTM! Main container configuration is well-structured.The container properly sources database credentials from the Secret, mounts the configuration from the ConfigMap, and executes the table creation command with the correct storage engine argument.
Note: The
imagePullPolicy: "Always"has been previously discussed and should ideally use{{ .Values.image.clpPackage.pullPolicy | quote }}for consistency.tools/deployment/package-helm/templates/_helpers.tpl (3)
53-79: LGTM! Image and probe helpers provide sensible defaults.The image reference helper correctly constructs the image tag with appropriate fallback to AppVersion. The probe timings use a reasonable strategy: aggressive readiness checks (6s initial, 2s period) for fast startup detection and conservative liveness checks (60s initial, 30s period) to prevent premature restarts.
Note: Past comments suggest documenting the probe timing rationale and potentially making these values configurable via values.yaml.
81-119: Node affinity logic implements OR fallback correctly.The
clp.createLocalPvhelper properly creates a local PersistentVolume with appropriate metadata and labels. The node affinity uses twonodeSelectorTerms(lines 113-118) which implement OR logic: the PV can schedule on nodes with either the specifiednodeRoleOR thecontrol-planerole. This provides a sensible fallback strategy.The use of the
Existsoperator is appropriate here because it checks for the presence of the role label, ensuring the PV binds to nodes with the correct role designation.Note: Past comments discuss making the
storageClassNameconfigurable for production use cases beyond local storage.
121-147: LGTM! PVC helper correctly implements label-based PV binding.The
clp.createPvchelper properly creates a PersistentVolumeClaim with label selectors that ensure it binds to the corresponding PersistentVolume created byclp.createLocalPv. The selector includes both the common selector labels and the component label for precise matching.Note: The
storageClassName: "local-storage"is hardcoded, consistent with the PV helper. Past comments discuss making this configurable for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
tools/deployment/package-helm/Chart.yaml(1 hunks)tools/deployment/package-helm/templates/_helpers.tpl(1 hunks)tools/deployment/package-helm/templates/database-data-pv.yaml(1 hunks)tools/deployment/package-helm/templates/database-logs-pv.yaml(1 hunks)tools/deployment/package-helm/templates/database-service.yaml(1 hunks)tools/deployment/package-helm/templates/db-table-creator-job.yaml(1 hunks)tools/deployment/package-helm/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
tools/deployment/package-helm/values.yaml
📚 Learning: 2025-08-19T14:41:28.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Applied to files:
tools/deployment/package-helm/values.yaml
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
tools/deployment/package-helm/values.yamltools/deployment/package-helm/Chart.yaml
📚 Learning: 2025-06-18T14:35:20.485Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.
Applied to files:
tools/deployment/package-helm/values.yaml
📚 Learning: 2025-06-18T20:39:05.899Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
tools/deployment/package-helm/Chart.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
tools/deployment/package-helm/templates/_helpers.tpl
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/db-table-creator-job.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/database-logs-pv.yaml
[error] 9-9: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
tools/deployment/package-helm/templates/database-data-pv.yaml
[error] 9-9: too many spaces inside braces
(braces)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
tools/deployment/package-helm/templates/database-service.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 9-9: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: rust-checks
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: package-image
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (9)
tools/deployment/package-helm/templates/database-logs-pv.yaml (1)
1-9: LGTM! Template correctly creates the database logs PV.The template properly invokes the
clp.createLocalPvhelper with appropriate parameters for database log storage. The 5Gi capacity and ReadWriteOnce access mode are reasonable defaults for development and testing scenarios.tools/deployment/package-helm/templates/database-data-pv.yaml (1)
1-9: LGTM! Database data PV template is correctly configured.The 20Gi capacity allocation for database data is appropriately larger than the 5Gi for logs. The template correctly invokes the helper with all required parameters.
tools/deployment/package-helm/values.yaml (2)
1-5: LGTM! Naming and access configuration is clear.The empty name overrides follow Helm conventions, and the
allowHostAccessForSbinScriptsflag with its explanatory comment provides good control over service exposure.
7-13: LGTM! Security context configuration follows best practices.Using non-root UIDs (1000) for both first-party and third-party components is appropriate. The separation allows for different security contexts if needed in future.
tools/deployment/package-helm/Chart.yaml (1)
1-17: LGTM! Chart metadata is complete and well-organised.The Chart.yaml follows Helm conventions with required fields properly ordered at the top. The metadata is comprehensive, including appropriate keywords, maintainer information, and links to project resources.
tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
1-20: LGTM! Job metadata and pod spec are properly configured.The Job configuration follows Kubernetes best practices with appropriate TTL cleanup (300s), OnFailure restart policy, and proper security context from values.
tools/deployment/package-helm/templates/_helpers.tpl (2)
1-31: LGTM! Naming helpers follow Helm conventions.The
clp.name,clp.fullname, andclp.charthelpers correctly implement standard Helm naming patterns with appropriate truncation (63 chars for DNS compatibility) and trailing dash handling.
33-51: LGTM! Label helpers implement Kubernetes recommended labels.The label helpers correctly separate immutable selector labels (name, instance) from mutable common labels (chart, version, managed-by), following Kubernetes best practices.
tools/deployment/package-helm/templates/database-service.yaml (1)
1-22: Unable to verify port name match — requires manual confirmation.The conditional service type and named port pattern follow Kubernetes best practices. However, I could not access the repository to confirm that the database StatefulSet container defines a port named
"database"to match this Service's targetPort.Action required: Verify that your database container port definition includes a matching named port:
ports: - name: database containerPort: 3306If this named port is missing or named differently, the Service will fail to route traffic to the database Pod.
| image: | ||
| clpPackage: | ||
| repository: "ghcr.io/y-scope/clp/clp-package" | ||
| pullPolicy: "IfNotPresent" | ||
| tag: "nightly" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the "nightly" tag for production deployments.
The "nightly" tag is appropriate for development and testing. However, for production deployments, users should override this with a specific stable version tag to ensure reproducibility and avoid unexpected updates.
Consider adding a comment above the tag field:
clpPackage:
repository: "ghcr.io/y-scope/clp/clp-package"
pullPolicy: "IfNotPresent"
+ # For production, override with a specific version tag (e.g., "v0.6.1")
tag: "nightly"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image: | |
| clpPackage: | |
| repository: "ghcr.io/y-scope/clp/clp-package" | |
| pullPolicy: "IfNotPresent" | |
| tag: "nightly" | |
| image: | |
| clpPackage: | |
| repository: "ghcr.io/y-scope/clp/clp-package" | |
| pullPolicy: "IfNotPresent" | |
| # For production, override with a specific version tag (e.g., "v0.6.1") | |
| tag: "nightly" |
🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around lines 15 to 19, the image
tag is set to "nightly" which is suitable for dev but dangerous for production;
add a comment above the tag explaining that "nightly" is for development/testing
only and instruct users to override this value with a pinned, stable
semantic-version tag for production deployments (and optionally show how to
override via Helm values or --set). Ensure the comment is concise and mentions
reproducibility and avoiding unexpected updates.
| database: | ||
| type: "mariadb" # "mariadb" or "mysql" | ||
| host: "localhost" | ||
| port: 30306 | ||
| name: "clp-db" |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Check ConfigMap template and database host usage
echo "=== Searching for ConfigMap template ==="
fd configmap.yaml tools/deployment/package-helm/templates/
echo -e "\n=== Examining ConfigMap template for database host ==="
cat tools/deployment/package-helm/templates/configmap.yaml
echo -e "\n=== Checking for any database service definitions ==="
rg -n "database" tools/deployment/package-helm/templates/ --type yaml -A 5 -B 2Repository: y-scope/clp
<shell_result>
Repository: y-scope/clp
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository y-scope/clp: Clone operation failed
</shell_result>
Kubernetes pod localhost vs service DNS database connection Helm values
<web_search_result>
Short answer
- Use localhost only when the database runs in the same Pod (same container or sidecar). For all other cases use the Service DNS name (or the short service name if in the same namespace). [1][2]
Why
- Kubernetes assigns DNS names to Services like ..svc.cluster.local and resolves them to the Service ClusterIP (routing to backing Pods). Pods in the same namespace may use the short name (service). Pod-local DNS exists only when the DB actually lives in the same Pod. [1][2]
Practical Helm/values pattern
-
values.yaml
db:
host: mysql # use "mysql" (same namespace) or "mysql.prod.svc.cluster.local"
port: 3306
user: app
password: secret -
Typical connection string (app config / template):
mysql://{{ .Values.db.user }}:{{ .Values.db.password }}@{{ .Values.db.host }}:{{ .Values.db.port }}/dbname -
If you need a local sidecar DB:
db:
host: localhost
port: 5432 -
If the DB lives in another namespace:
db:
host: mysql.prod.svc.cluster.local
Best practices / gotchas
- Prefer Service DNS (service.namespace.svc.cluster.local) for separation, scaling and restarts. [1]
- Ensure proper readiness/liveness and that app starts only after DB is reachable (initContainer, retries, or Kubernetes StartupProbe).
- For stateful databases use a Service (ClusterIP or headless for direct pod addresses) and persistent volumes; headless services return pod IPs (not a single ClusterIP) if you need per-pod addressing. [1]
- Use Helm value overrides per-environment (values-dev.yaml, values-prod.yaml) or --set when deploying.
References
- Kubernetes: DNS for Services and Pods. [1]
- Kubernetes: Debug Services / how to test service DNS from a Pod. [2]
</web_search_result>
Database host "localhost" is incompatible with Kubernetes service architecture.
The host: "localhost" configuration is only appropriate when the database runs as a sidecar or in the same Pod. For Kubernetes deployments with separate database services, use the Service DNS name instead (e.g., service name or service.namespace.svc.cluster.local).
Additionally, the port 30306 (a NodePort range) combined with "localhost" suggests a configuration mismatch—either the database runs in the same Pod (use "localhost" and remove the NodePort), or it runs as a separate service (use Service DNS and appropriate port).
🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around lines 26 to 30, the database
host is set to "localhost" and port 30306 which is incompatible with a separate
Kubernetes Service; update host to the DB Service DNS (for example use
"<service-name>" or "service.namespace.svc.cluster.local") and set port to the
Service's target port (commonly 3306) so the app talks to the cluster service,
or if the DB is truly a sidecar in the same Pod keep "localhost" and
change/remove the NodePort usage (30306) so the port reflects the container's
internal MySQL port.
| {{/* | ||
| Creates a PersistentVolumeClaim. | ||
|
|
||
| @param {object} root Root template context | ||
| @param {string} component Component label | ||
| @param {string} capacity Storage capacity | ||
| @param {string[]} accessModes | ||
| @return {string} YAML-formatted PersistentVolumeClaim resource | ||
| */}} | ||
| {{- define "clp.createPvc" -}} | ||
| apiVersion: "v1" | ||
| kind: "PersistentVolumeClaim" | ||
| metadata: | ||
| name: {{ include "clp.fullname" .root }}-{{ .component }} | ||
| labels: | ||
| {{- include "clp.labels" .root | nindent 4 }} | ||
| app.kubernetes.io/component: {{ .component | quote }} | ||
| spec: | ||
| accessModes: {{ .accessModes }} | ||
| storageClassName: "local-storage" | ||
| selector: | ||
| matchLabels: | ||
| {{- include "clp.selectorLabels" .root | nindent 6 }} | ||
| app.kubernetes.io/component: {{ .component | quote }} | ||
| resources: | ||
| requests: | ||
| storage: {{ .capacity }} | ||
| {{- end }} |
There was a problem hiding this comment.
I guess this helper is for a future PR? Could we introduce it in that PR instead?
There was a problem hiding this comment.
Right. it should be used in Deployments (TBA) rather than StatefulSets. it should be removed then
| - matchExpressions: | ||
| - key: "node-role.kubernetes.io/control-plane" | ||
| operator: "Exists" |
There was a problem hiding this comment.
Why do the volume have to be on the control-plane node(s)? Or am I misunderstanding something?
There was a problem hiding this comment.
Here's the flow of how a Pod (StatefulSet / Job / Deployment) is scheduled on a node:
- A PV is created with nodeAffinity that restricts which nodes can use it.
- A PVC can then bind to the PV.
- The Pod that uses the PVC is scheduled onto the same node.
This is to help users who can only set up a single-node cluster (when not using kind). They may only be able to set up a control-plane node, then:
- Say we are calling this helper with "nodeRole=worker".
- A PV specifies nodeAffinity for nodes with role "worker", but since no such node exists, it falls back to the only node with the control-plane role.
- A PVC can then bind to the PV on this node.
- The Pod that uses the PVC is scheduled onto the one-and-only control-plane node.
| @@ -0,0 +1,4 @@ | |||
| chart-dirs: | |||
| - "tools/deployment" | |||
| chart-yaml-schema: "build/toolchains/helm/etc/chart_schema.yaml" | |||
There was a problem hiding this comment.
As discussed offline, can we pass this as a command-line argument that uses the taskfile variable G_BUILD_DIR?
There was a problem hiding this comment.
right, sorry for missing this in the last round
There was a problem hiding this comment.
when i created the PR, i initially tried to set up ct without the ct.yaml but this didn't work
ct lint \
--chart-dirs="{{.ROOT_DIR}}/tools/deployment" \
--chart-yaml-schema="{{.G_HELM_TOOLCHAIN_DIR}}/etc/chart_schema.yaml" \
--lint-conf="{{.ROOT_DIR}}/.yamllint.yaml"
would yield
Linting charts...
Directory "tools/deployment/package-helm" is not a valid chart directory. Skipping...
Directory "tools/deployment/package-helm" is not a valid chart directory. Skipping...
with the belief that their arg parser should not process the config args differently than their yaml config parser, i finally figured out it's because they don't support absolute paths in in --chart-dirs. not sure if it's a bug...
so this works
ct lint \
--chart-dirs="tools/deployment" \
--chart-yaml-schema="{{.G_HELM_TOOLCHAIN_DIR}}/etc/chart_schema.yaml" \
--lint-conf=".yamllint.yaml"
we can remove the ct.yaml altogether then
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/deployment/package-helm/templates/db-table-creator-job.yaml (1)
23-23: Pin the busybox image version.Using
busybox:latestintroduces unpredictability across deployments. Different versions may have varying behaviour or available utilities.Apply this diff to pin to a stable version:
- image: "busybox:latest" + image: "busybox:1.37"tools/deployment/package-helm/templates/_helpers.tpl (1)
122-122: Consider usingtoYamlfor safer access modes rendering.Directly rendering
accessModeswith{{ .accessModes }}assumes callers pass a properly formatted YAML list string. If callers pass a Go slice or the format varies, this could produce invalid YAML.Apply this diff for more robust rendering:
- accessModes: {{ .accessModes }} + accessModes: + {{- toYaml .accessModes | nindent 4 }}This ensures correct YAML formatting regardless of the input type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
taskfiles/lint.yaml(4 hunks)taskfiles/toolchains.yaml(2 hunks)tools/deployment/package-helm/templates/_helpers.tpl(1 hunks)tools/deployment/package-helm/templates/db-table-creator-job.yaml(1 hunks)tools/deployment/package-helm/test.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
📚 Learning: 2024-11-18T16:49:20.248Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-08-25T06:29:59.610Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.610Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-05-06T09:48:55.408Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-08-17T15:26:22.790Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/utils/config.py:0-0
Timestamp: 2025-08-17T15:26:22.790Z
Learning: In the CLP project, Yoda-style conditions (literal on the left) are only used for `== 0` checks, not for other comparison operators like `>` or `<`. This is specifically to catch accidental assignment operators (writing `=` instead of `==`) in equality comparisons against zero.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-07-08T17:20:02.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-10-27T04:05:01.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1507
File: taskfiles/deps/lock.yaml:66-76
Timestamp: 2025-10-27T04:05:01.560Z
Learning: In the CLP project, the `uv` CLI tool is documented as a prerequisite in docs/src/dev-docs/building-package.md and is not installed via a task dependency like the rust toolchain. UV is expected to be available in the environment rather than being provisioned by the taskfile system.
Applied to files:
taskfiles/toolchains.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
tools/deployment/package-helm/templates/_helpers.tpltaskfiles/lint.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-11-17T22:58:50.056Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-11-17T22:58:50.056Z
Learning: In the y-scope/clp repository, when enabling new linting tools (ruff, mypy) on Python components, the team uses an incremental approach: first enable the tools with errors allowed (exit code 0), apply only safe auto-fixable fixes, then address remaining issues in follow-up PRs. During the initial enablement PR, reviews should focus on correctness of auto-fixes rather than flagging new code quality issues.
Applied to files:
taskfiles/lint.yaml
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
taskfiles/lint.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/db-table-creator-job.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: syntax error: expected , but found ''
(syntax)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 20-20: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 45-45: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 58-58: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
[error] 63-63: too many spaces inside braces
(braces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks
- GitHub Check: build (macos-15)
🔇 Additional comments (8)
tools/deployment/package-helm/test.sh (2)
1-7: Proper error handling and shebang implemented.The script now includes the recommended Bash best practices with
#!/usr/bin/env bashand strict error handling (set -o errexit,set -o nounset,set -o pipefail), which aligns with the previous feedback and CLP project conventions for fail-fast behaviour.
13-29: Rest of script looks good.The Kind cluster configuration, Helm operations, and cleanup logic are sound. The 2-second delay on line 28 provides adequate time for resource teardown before reinstalling the chart.
taskfiles/toolchains.yaml (4)
52-55: Environment file generation looks correct.The here-doc delimiter is properly quoted as
<<"EOF"to prevent variable expansion in the created file, ensuring$PATHis preserved for runtime evaluation. Implementation aligns with prior feedback.
42-42: I'm unable to proceed with verification due to system limitations preventing script execution. The repository cloning has failed consistently, and I cannot execute external commands to download and inspect the chart-testing tarball.Unable to verify
etc/chart_schema.yamlpath in chart-testing tarball due to execution constraints.Without access to either the repository codebase or the ability to download and inspect the chart-testing 3.14.0 tarball, I cannot confirm whether the file path "etc/chart_schema.yaml" is correct or present in the release. Manual verification of the tarball contents or inspection of the repository files would be required to validate this concern.
4-4: Unable to verify remote.yaml task signature due to repository access limitations.The review comment requests verification that
remote:download-and-extract-taraccepts the parameters passed (OUTPUT_DIR, URL, FILE_SHA256, INCLUDE_PATTERNS, NUM_COMPONENTS_TO_STRIP). However, the repository could not be cloned in the sandbox environment, and no public documentation exists for the yscope-dev-utils utilities module.Manual verification is required: confirm that the remote.yaml file exists at
tools/yscope-dev-utils/exports/taskfiles/utils/remote.yamland that thedownload-and-extract-tartask accepts all five parameters with compatible signatures.
24-24: Based on my verification, I have found a critical factual error in the review comment:The review comment incorrectly states that darwin/amd64 (Intel macOS) is missing from chart-testing v3.14.0.
Here's what I verified:
- chart-testing v3.14.0 was released on October 8, 2025 and is a valid, current release
- darwin/amd64 binary DOES exist as
chart-testing_3.14.0_darwin_amd64.tar.gz- All required platforms are available: linux/amd64, linux/arm64, darwin/amd64, and darwin/arm64
The available binaries for v3.14.0 include:
chart-testing_3.14.0_darwin_amd64.tar.gz✓chart-testing_3.14.0_darwin_arm64.tar.gz✓chart-testing_3.14.0_linux_amd64.tar.gz✓chart-testing_3.14.0_linux_arm64.tar.gz✓- Plus additional variants for armv6, armv7, ppc64le, s390x, and windows
Version 3.14.0 is a valid release with full platform support, including darwin/amd64. Confirm that the SHA256 checksums in lines 32-41 match the official GitHub release assets.
Likely an incorrect or invalid review comment.
taskfiles/lint.yaml (2)
119-129: Yamllint paths correctly exclude Helm charts.The updated paths at lines 119-129 intentionally exclude
tools/deployment/package-helm, routing Helm chart YAML linting throughct lint(line 70) instead. This separation of concerns is appropriate: chart-testing handles Helm-specific validation, while yamllint handles general YAML files.
59-73: The repository clone operation failed, which prevents me from verifying the file paths directly. Without access to the codebase, I cannot confirm:
- Whether Helm charts exist in
tools/deployment- Whether
.yamllint.yamlexists at the repository root- How
{{.G_HELM_TOOLCHAIN_DIR}}/etc/chart_schema.yamlresolves after sourcing the environment file
Verify Helm chart directory and configuration file paths.
The helm task at lines 71–73 specifies
--chart-dirs="tools/deployment",--chart-yaml-schema="{{.G_HELM_TOOLCHAIN_DIR}}/etc/chart_schema.yaml", and--lint-conf=".yamllint.yaml". Confirm that:
- Actual Helm charts are located within
tools/deployment(not a subdirectory)- The path
{{.G_HELM_TOOLCHAIN_DIR}}/etc/chart_schema.yamlresolves correctly after sourcing the environment file- The
.yamllint.yamlfile exists at the repository root
| kind delete cluster --name clp-test | ||
| rm -rf /tmp/clp | ||
| mkdir -p /tmp/clp/var/{data,log}/database |
There was a problem hiding this comment.
Add error handling to kind delete for idempotency.
Line 9 (kind delete cluster --name clp-test) will cause the script to exit if the cluster doesn't exist (on first run), making the script non-idempotent. This is inconsistent with line 27, which uses || true for the equivalent helm uninstall command. For a local test script that developers will run multiple times, this should be idempotent.
Apply this diff to make the script idempotent:
-kind delete cluster --name clp-test
+kind delete cluster --name clp-test || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kind delete cluster --name clp-test | |
| rm -rf /tmp/clp | |
| mkdir -p /tmp/clp/var/{data,log}/database | |
| kind delete cluster --name clp-test || true | |
| rm -rf /tmp/clp | |
| mkdir -p /tmp/clp/var/{data,log}/database |
🤖 Prompt for AI Agents
In tools/deployment/package-helm/test.sh around lines 9 to 11, the script calls
"kind delete cluster --name clp-test" which will error and exit if the cluster
doesn't exist; make this idempotent by adding error-tolerant handling. Replace
the direct delete with a safe form (e.g., run the delete command with a trailing
"|| true" or guard it with a check like "if kind get clusters | grep -q
'^clp-test$'; then kind delete cluster --name clp-test; fi") so the script
continues without failing when the cluster is absent; keep the subsequent rm -rf
and mkdir lines unchanged.
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title & body, how about:
feat(deployment): Set up initial Helm chart for CLP package:
- Add `StatefulSet` for `database` component and `db-table-creator` job.
- Introduce `lint:helm` task to lint the chart using `chart-testing` (`ct`).
- Update CI workflow to include Helm chart linting.
lint:helmtask to lint the chart using chart-testing (ct).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 adds a Helm chart for deploying the CLP package. It also introduces Helm chart linting and adds it to the CI pipeline.
Changes:
tools/deployment/package-helm/) for CLP package deployment, including templates for ConfigMaps, Secrets, Services, StatefulSets, Jobs, and PersistentVolumes.ct lint) to theclp-lint.yamlworkflow and thelint.yamltaskfile.toolchains.yamlto install thect(chart testing) tool.chart_schema.yamlto define the structure for HelmChart.yamlfiles andct.yamlto configure the chart testing tool (ct).Checklist
breaking change.
Validation performed
./test.sh.kubectl get pods -wand observed all pods were started successfully. the db-table-creator-job ran after thedatabase-statefulsetwas ready in around 1 minute.Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.