Skip to content

fix(helm): Replace database.name with database.names.clp (fixes #1811).#1814

Merged
junhaoliao merged 2 commits into
y-scope:mainfrom
junhaoliao:k8s-db-name
Dec 19, 2025
Merged

fix(helm): Replace database.name with database.names.clp (fixes #1811).#1814
junhaoliao merged 2 commits into
y-scope:mainfrom
junhaoliao:k8s-db-name

Conversation

@junhaoliao

@junhaoliao junhaoliao commented Dec 19, 2025 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

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 change aligns with the CLP package configuration structure where database names are organized under a names object, matching the structure used in clp-config.yaml.

Checklist

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

Validation performed

cd tools/deployment/package-helm
./test.sh

# observed "All jobs completed and services are ready."

Summary by CodeRabbit

Release Notes

  • Chores
    • Bumped deployment chart version to 0.1.2-dev.8.
    • Updated database configuration structure in deployment templates to use a nested naming convention, improving configuration clarity.

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

@junhaoliao junhaoliao requested a review from a team as a code owner December 19, 2025 04:08
@coderabbitai

coderabbitai Bot commented Dec 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

The pull request restructures the Helm chart's database configuration from a flat name field to a nested names.clp structure across values and templates, with a corresponding chart version increment.

Changes

Cohort / File(s) Summary
Chart version
tools/deployment/package-helm/Chart.yaml		 Bumps chart version from 0.1.2-dev.7 to 0.1.2-dev.8
Database configuration templates
tools/deployment/package-helm/templates/configmap.yaml, tools/deployment/package-helm/templates/database-statefulset.yaml		 Updates database name source from .Values.clpConfig.database.name to .Values.clpConfig.database.names.clp in both clp-config.yaml and webui-server-settings.json entries
Configuration values
tools/deployment/package-helm/values.yaml		 Restructures clpConfig.database from flat name: "clp-db" to nested names.clp: "clp-db" structure

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Changes follow a consistent, mechanical pattern across all files
  • Configuration restructuring only—no logic or control flow modifications
  • Straightforward find-and-replace style updates with no conditional logic or edge cases to evaluate

Possibly related issues

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: replacing database.name with database.names.clp in the Helm configuration, which is accurately reflected across all four modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a61596d and b9e4773.

📒 Files selected for processing (4)
  • tools/deployment/package-helm/Chart.yaml (1 hunks)
  • tools/deployment/package-helm/templates/configmap.yaml (2 hunks)
  • tools/deployment/package-helm/templates/database-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/values.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T05:13:13.298Z 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:217-223
Timestamp: 2025-09-25T05:13:13.298Z
Learning: The compression scheduler service in CLP runs with CLP_UID_GID (current user's UID:GID) rather than CLP_SERVICE_CONTAINER_UID_GID (999:999), unlike infrastructure services such as database, queue, redis, and results cache which run with the service container UID:GID.

Applied to files:

  • tools/deployment/package-helm/values.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:

  • tools/deployment/package-helm/values.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/database-statefulset.yaml

[error] 36-36: too many spaces inside braces

(braces)

[error] 36-36: 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). (4)
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: check-generated
  • GitHub Check: antlr-code-committed (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (5)
tools/deployment/package-helm/Chart.yaml (1)

3-3: LGTM! Appropriate version increment.

The chart version bump correctly reflects the structural change to the database configuration schema.

tools/deployment/package-helm/templates/database-statefulset.yaml (1)

36-36: LGTM! Reference correctly updated.

The database name reference has been properly migrated to use the new nested names.clp path. The YAMLlint warning about braces is a false positive—the spacing is standard Helm template syntax.

tools/deployment/package-helm/templates/configmap.yaml (2)

33-34: LGTM! Configuration structure correctly updated.

The clp-config.yaml database section now uses the nested names.clp structure, properly aligning with the CLP package configuration format.

132-132: LGTM! WebUI configuration correctly updated.

The SqlDbName field in webui-server-settings.json properly references the new nested path, maintaining consistency with the configuration structure changes.

tools/deployment/package-helm/values.yaml (1)

40-41: LGTM! All references successfully migrated.

The nested names.clp structure correctly prepares for potential multiple database configurations and aligns with the CLP package configuration format. Verification confirms no remaining references to the old database.name path in Helm templates and values.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@junhaoliao junhaoliao changed the title fix(helm): Replace database.name with database.names.clp (fixes #… fix(helm): Replace database.name with database.names.clp (fixes #1811). Dec 19, 2025
@junhaoliao junhaoliao merged commit 65b3c7c into y-scope:main Dec 19, 2025
44 of 48 checks passed
@junhaoliao junhaoliao mentioned this pull request Dec 26, 2025
Closed
16 tasks
davidlion pushed a commit to davidlion/clp that referenced this pull request Jan 17, 2026
@junhaoliao @davidlion
fix(helm): Replace database.name with database.names.clp (fixes y…
69e2bb9 
…-scope#1811). (y-scope#1814)
@junhaoliao junhaoliao deleted the k8s-db-name branch May 7, 2026 19:46
junhaoliao added a commit to junhaoliao/clp that referenced this pull request May 17, 2026
@junhaoliao
fix(helm): Replace database.name with database.names.clp (fixes y…
4751813 
…-scope#1811). (y-scope#1814)
junhaoliao added a commit to junhaoliao/clp that referenced this pull request May 17, 2026
@junhaoliao @davidlion
fix(helm): Replace database.name with database.names.clp (fixes y…
5720eb5 
…-scope#1811). (y-scope#1814)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sitaowang1998 sitaowang1998 sitaowang1998 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@junhaoliao @sitaowang1998