refactor(helm): Refactor createLocalPv:#1771
Conversation
…gory` and `name` usage; correct hostPath to match actual path.
WalkthroughThe Helm chart version is bumped from 0.1.2-dev.3 to 0.1.2-dev.4. The helper template Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
| mountPath: "/etc/mysql/conf.d/logging.cnf" | ||
| subPath: "mysql-logging.cnf" | ||
| readOnly: true | ||
| - name: "database-data" |
There was a problem hiding this comment.
these lines are unrelated to the main purpose of the PR. they're only updated to avoid code style violations (unsorted list items)
| mountPath: "/etc/redis/redis.conf" | ||
| subPath: "redis.conf" | ||
| readOnly: true | ||
| - name: "redis-data" |
There was a problem hiding this comment.
these lines are unrelated to the main purpose of the PR. they're only updated to avoid code style violations (unsorted list items)
| mountPath: "/etc/mongo/mongod.conf" | ||
| subPath: "mongod.conf" | ||
| readOnly: true | ||
| - name: "results-cache-data" |
There was a problem hiding this comment.
these lines are unrelated to the main purpose of the PR. they're only updated to avoid code style violations (unsorted list items)
component_categoryandname usage; correct hostPath to match actual path.createLocalPv to standardize component_categoryandname usage; correct hostPath to match actual path.
createLocalPv to standardize component_categoryandname usage; correct hostPath to match actual path.createLocalPv to standardize component_category and name usage; correct hostPath to match actual path.
createLocalPv to standardize component_category and name usage; correct hostPath to match actual path.createLocalPv to compose metadata.name from name; Rename parameter component -> component_category.
createLocalPv to compose metadata.name from name; Rename parameter component -> component_category.createLocalPv to compose metadata.name from name; Rename parameter component -> component_category; Fix typo in hostPath specifications.
| "root" . | ||
| "name" (printf "%s-results-cache-data" (include "clp.fullname" .)) | ||
| "component" "results-cache" | ||
| "component_category" "results-cache" |
There was a problem hiding this comment.
I thought we were going to use underscores even in the service name? #1749 (comment)
There was a problem hiding this comment.
do note that metadata.name and label values requires the names to be RFC1123-compliant (RFC1123 refers to RFC952 for host naming), which forbids _:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
This means the name must:
- contain at most 63 characters
- contain only lowercase alphanumeric characters or '-'
- start with an alphabetic character
- end with an alphanumeric character
i do think the inconsistencies would cause confusions in the long term. We should start changing the _ in service names into - in the CLP Package code and the Docker Compose project. @coderabbitai please create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track standardizing service names from underscores to hyphens
gh issue create \
--title "Standardise service names to use hyphens instead of underscores for RFC1123 compliance" \
--body "## Background
Kubernetes metadata.name and label values require RFC1123-compliant names, which only allow lowercase alphanumeric characters and hyphens (not underscores). This creates an inconsistency with current service names in the CLP Package code and Docker Compose project that use underscores.
## Objective
Standardise service names across the codebase to use hyphens (\`-\`) instead of underscores (\`_\`) in:
- CLP Package code
- Docker Compose project
This will ensure consistency with Kubernetes naming requirements and prevent confusion in the long term.
## References
- Related PR: https://github.com/y-scope/clp/pull/1771
- Discussion: https://github.com/y-scope/clp/pull/1771#discussion_r2618412522
- Kubernetes naming requirements: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #1777 to track standardising service names from underscores to hyphens for RFC1123 compliance across the CLP Package code and Docker Compose project. The issue has been assigned to you.
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/templates/_helpers.tpl(2 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). (4)
- GitHub Check: package-image
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
tools/deployment/package-helm/templates/_helpers.tpl (2)
101-102: Documentation is clear and consistent with implementation.The parameter documentation accurately reflects the refactored parameter names and provides helpful examples. The format matches the suggestion from the past review comment.
118-118: Label update is consistent with parameter rename.The
app.kubernetes.io/componentlabel now correctly usescomponent_category, maintaining consistency with the refactored parameter name.
| kind: "PersistentVolume" | ||
| metadata: | ||
| name: {{ .name }} | ||
| name: {{ include "clp.fullname" .root }}-{{ .component_category }}-{{ .name }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for upgrade, migration, or breaking change documentation
rg -i -C 3 "upgrade|migration|breaking.*change|persistent.*volume.*name" --type=mdRepository: y-scope/clp
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Search for CHANGELOG files and related version documentation
find . -iname "CHANGELOG*" -o -iname "RELEASE*" -o -iname "MIGRATION*" | head -20Repository: y-scope/clp
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the specific file and surrounding context for PV-related comments
cat -n tools/deployment/package-helm/templates/_helpers.tpl | sed -n '100,130p'Repository: y-scope/clp
Length of output: 1383
🏁 Script executed:
#!/bin/bash
# Look for chart metadata and version information
cat tools/deployment/package-helm/Chart.yaml 2>/dev/null || cat tools/deployment/package-helm/*/Chart.yaml 2>/dev/nullRepository: y-scope/clp
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Search for values.yaml to understand component_category usage
find tools/deployment/package-helm -name "values*.yaml" -type f -exec head -50 {} \;Repository: y-scope/clp
Length of output: 1279
🏁 Script executed:
#!/bin/bash
# Check if CHANGELOG exists and display its contents
if [ -f "CHANGELOG.md" ]; then
cat CHANGELOG.md | head -100
else
echo "CHANGELOG.md not found"
fi
# Also check for other common patterns
ls -la | grep -i "change\|release\|news"Repository: y-scope/clp
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Search for any comments or notes in the helm templates about PV naming or breaking changes
rg -C 5 "component_category\|createLocalPv" --type=yaml --type=tplRepository: y-scope/clp
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check git log for recent changes to this specific line/file
cd tools/deployment/package-helm && git log --oneline -20 -- templates/_helpers.tplRepository: y-scope/clp
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Look for any PR or issue references in comments within the chart files
rg "#\d{4}" tools/deployment/package-helm/ | head -20Repository: y-scope/clp
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the git commit that introduced this change
git show 6af8ac3 --statRepository: y-scope/clp
Length of output: 50367
🏁 Script executed:
#!/bin/bash
# Look for all uses of createLocalPv helper to understand the impact
rg "createLocalPv" tools/deployment/package-helm/Repository: y-scope/clp
Length of output: 843
🏁 Script executed:
#!/bin/bash
# Check for any PersistentVolume or PVC related files
find tools/deployment/package-helm -type f | xargs rg -l "PersistentVolume\|PersistentVolumeClaim" | head -10Repository: y-scope/clp
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Search for documentation files in the Helm chart directory
find tools/deployment/package-helm -type f -name "*.md" -o -name "README*" | xargs cat 2>/dev/null | head -200Repository: y-scope/clp
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look for any values or template that shows example PV names or migration info
rg -i "database-data|redis-data|archives-data|pv.*name|metadata.*name" tools/deployment/package-helm/ --type=yamlRepository: y-scope/clp
Length of output: 438
Document the breaking change to PersistentVolume naming and provide an upgrade path.
Changing the metadata.name composition adds {{ .component_category }} to PV names, which generates different names when upgrading existing Helm releases. Since persistentVolumeReclaimPolicy is set to "Retain", old PVs will remain as orphaned resources requiring manual cleanup. PVCs must be rebound or manually migrated to the new PV names.
Add upgrade notes or a migration guide to the chart documentation explaining this breaking change, especially for non-development deployments.
🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/_helpers.tpl around line 115, the
metadata.name change (adding {{ .component_category }}) breaks existing
PersistentVolume naming on upgrade and can orphan PVs due to reclaimPolicy:
Retain; update the chart docs and release notes to clearly describe this
breaking change and provide a step-by-step upgrade/migration path: (1) document
how to detect old PVs and their PVCs, (2) provide safe steps to migrate bindings
(example steps: scale down workloads, snapshot/backup data, edit PV.claimRef or
recreate PVs with the expected names and correct spec, rebind PVCs, and then
resume workloads), (3) recommend a pre-upgrade checklist (backup, downtime
window), (4) add a chart migration/notes.md or NOTES.txt entry with exact
commands and expected outcomes, and (5) consider adding an opt-in compatibility
flag to preserve the old name format for one release to allow safe migration.
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title & body, how about:
refactor(helm): Refactor `createLocalPv`:
- Compose `metadata.name` from `name`.
- Rename `component` to `component_category`.
- Fix typo in `hostPath` specifications.
createLocalPv to compose metadata.name from name; Rename parameter component -> component_category; Fix typo in hostPath specifications.createLocalPv:
createLocalPv:createLocalPv:…
createLocalPv:…createLocalPv: …
createLocalPv: …createLocalPv:
| "capacity" "20Gi" | ||
| "accessModes" (list "ReadWriteOnce") | ||
| "hostPath" (printf "%s/results-cache" .Values.clpConfig.data_directory) | ||
| "hostPath" (printf "%s/results_cache" .Values.clpConfig.data_directory) |
There was a problem hiding this comment.
sorry, the mkdir directory should also be updated but was missed:
results_cache
-> results_cache
@coderabbitai create an issue to track
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the test.sh file to verify the directory naming issue
cat tools/deployment/package-helm/test.sh
Length of output: 668
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track the directory naming inconsistency in test.sh
gh issue create \
--repo y-scope/clp \
--title "Update test.sh to use results_cache directory naming" \
--body "In PR #1771, the hostPath for results-cache PV was updated from \`results-cache\` to \`results_cache\` (dash to underscore), but the corresponding directory creation in \`tools/deployment/package-helm/test.sh\` line 11 was not updated.
Currently, test.sh creates:
\`\`\`bash
mkdir -p /tmp/clp/var/{data,log}/{database,queue,redis,results-cache}
\`\`\`
It should be updated to:
\`\`\`bash
mkdir -p /tmp/clp/var/{data,log}/{database,queue,redis,results_cache}
\`\`\`
**Context:**
- PR: https://github.com/y-scope/clp/pull/1771
- Comment: https://github.com/y-scope/clp/pull/1771#discussion_r2621405422
- Reported by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #1782 to track updating the directory naming in test.sh from results-cache to results_cache to match the hostPath changes in this PR.
🧠 Learnings used
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.
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 1170
File: tools/deployment/presto-clp/scripts/generate-metadata-filter-file.py:0-0
Timestamp: 2025-09-04T12:26:54.788Z
Learning: The `set-up-config.sh` script in tools/deployment/presto-clp/scripts/ ensures that the output directory for metadata-filter.json already exists before calling generate-metadata-filter-file.py, so directory creation in the Python script is not strictly necessary.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.436Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
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>
componenttocomponent_category.metadata.namefromnameandcomponent_category.hostPathspecifications.Important
This PR is created address handling / styling issues found in reviewing #1749 . It should be merged:
createLocalPv: #1771).See Update test.sh to use results_cache directory naming #1782 for details of the bug.
waitForhelper leveraging newjob-watcherservice account privileges. #1772 so that the Helm chart development version numbers will be correctly incremented.createLocalPv: #1771 are incremental to reach the state in feat(helm): Add compression & query job orchestration and the webui to the chart. #1749Description
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.
Update the PersistentVolume helper to rename parameter
component->component_category. Thenameparameter is now also used only as a part, rather than a whole, to construct themetadata.nameof the PersistentVolume. Typos, such as using-instead of_, inhostPathspecifications are also correctedChecklist
breaking change.
Validation performed
same validation steps as done in #1698
Summary by CodeRabbit
Release Notes
Chores
Refactor
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.