Openhab - #969
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:openhab
📝 WalkthroughWalkthroughThe openHAB chart now supports shared or dedicated PVCs with centralized claim settings, configurable relative subpaths, validation, subpath initialization, backup handling, deduplicated volume declarations, and updated documentation and tests. ChangesopenHAB persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Values as Helm values
participant Validation as openhab.validatePersistence
participant Templates as PVC and StatefulSet templates
participant InitContainer as persistence init container
participant Pod as openHAB pod
Values->>Validation: Supply claim and subPath settings
Validation->>Templates: Validate and resolve claims
Templates->>InitContainer: Render required persistence mounts
InitContainer->>Pod: Create configured subdirectories
Templates->>Pod: Mount shared or dedicated claims at subPaths
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/openhab/templates/backup-cronjob.yaml`:
- Around line 49-59: Update the backup CronJob pod template by adding a writable
init container before the backup container that mounts each selected PVC at its
root and runs mkdir -p for the configured userdata and conf subPaths. Ensure it
handles only enabled backup volumes, while preserving the existing read-only
subPath mounts in the backup container.
In `@charts/openhab/values.schema.json`:
- Around line 218-236: Update the persistence schema and chart upgrade path so
legacy per-directory settings under persistence.userdata, persistence.conf, and
persistence.addons are not accepted and ignored; either reject them through
template validation or explicitly migrate them into persistence.claim. Ensure
existing releases cannot silently fall back to the claim default of 5Gi, and
document the required upgrade procedure before release.
In `@charts/openhab/values.yaml`:
- Around line 116-141: Update charts/openhab/values.yaml lines 116-141 and
charts/openhab/README.md lines 139-150 to describe userdata, conf, and addons as
mounting their resolved or selected PVC, covering both shared and separate
existing-claim modes. Update charts/openhab/templates/NOTES.txt lines 77-97 to
derive the displayed label from the resolved claim mode and omit
persistence.claim.size when an existing claim is used; preserve capacity output
for chart-generated claims.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff65f1ed-54b4-479f-b1d3-11a2bfdb7aa4
📒 Files selected for processing (12)
charts/openhab/README.mdcharts/openhab/docs/storage.mdcharts/openhab/templates/NOTES.txtcharts/openhab/templates/_helpers.tplcharts/openhab/templates/backup-cronjob.yamlcharts/openhab/templates/pvc.yamlcharts/openhab/templates/statefulset.yamlcharts/openhab/tests/pvc_test.yamlcharts/openhab/tests/statefulset_test.yamlcharts/openhab/tests/validation_test.yamlcharts/openhab/values.schema.jsoncharts/openhab/values.yaml
| "claim": { | ||
| "type": "object", | ||
| "description": "Settings for the generated data PVC", | ||
| "properties": { | ||
| "storageClass": { | ||
| "type": "string" | ||
| }, | ||
| "accessMode": { | ||
| "type": "string", | ||
| "enum": ["ReadWriteOnce", "ReadWriteMany", "ReadOnlyMany"], | ||
| "default": "ReadWriteOnce" | ||
| }, | ||
| "size": { | ||
| "type": "string", | ||
| "pattern": "^[0-9]+(Mi|Gi|Ti|M|G|T)$", | ||
| "default": "5Gi" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent silent loss of per-directory storage settings on upgrade.
The schema still accepts persistence.userdata, persistence.conf, and persistence.addons storage settings. The generated PVC reads only persistence.claim. Existing release values can therefore be accepted but ignored, and the chart can provision the default 5Gi claim instead.
Reject the obsolete settings with template validation, or migrate them explicitly. Add an upgrade procedure before release. Do not silently replace these settings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/openhab/values.schema.json` around lines 218 - 236, Update the
persistence schema and chart upgrade path so legacy per-directory settings under
persistence.userdata, persistence.conf, and persistence.addons are not accepted
and ignored; either reject them through template validation or explicitly
migrate them into persistence.claim. Ensure existing releases cannot silently
fall back to the claim default of 5Gi, and document the required upgrade
procedure before release.
| # -- userdata: Runtime state, JSONDB, logs, persistence data (REQUIRED) | ||
| userdata: | ||
| # -- Mount userdata from the shared PVC | ||
| enabled: true | ||
| # -- Existing PVC for userdata (use this, conf.existingClaim and addons.existingClaim together for separate PVCs) | ||
| existingClaim: "" | ||
| # -- Subdirectory within the shared PVC to mount at /openhab/userdata | ||
| subPath: userdata | ||
|
|
||
| # -- conf: Configuration files synced before startup (REQUIRED) | ||
| conf: | ||
| # -- Enable PVC for conf | ||
| # -- Mount conf from the shared PVC | ||
| enabled: true | ||
| # -- Storage class name (empty = cluster default) | ||
| storageClass: "" | ||
| # -- Access mode | ||
| accessMode: ReadWriteOnce | ||
| # -- Storage size | ||
| size: 1Gi | ||
| # -- Use an existing PVC instead of creating one | ||
| # -- Existing PVC for conf (use this, userdata.existingClaim and addons.existingClaim together for separate PVCs) | ||
| existingClaim: "" | ||
| # -- Subdirectory within the shared PVC to mount at /openhab/conf | ||
| subPath: conf | ||
|
|
||
| # -- addons: Drop-in JAR addons not available via marketplace (optional) | ||
| addons: | ||
| # -- Enable PVC for addons | ||
| # -- Mount addons from the shared PVC | ||
| enabled: true | ||
| # -- Storage class name (empty = cluster default) | ||
| storageClass: "" | ||
| # -- Access mode | ||
| accessMode: ReadWriteOnce | ||
| # -- Storage size | ||
| size: 2Gi | ||
| # -- Use an existing PVC instead of creating one | ||
| # -- Existing PVC for addons (use this, userdata.existingClaim and conf.existingClaim together for separate PVCs) | ||
| existingClaim: "" | ||
| # -- Subdirectory within the shared PVC to mount at /openhab/addons | ||
| subPath: addons |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe the resolved PVC mode correctly.
The chart supports separate existing claims, but these texts always describe a shared PVC. The NOTES output can also report persistence.claim.size for an external claim whose capacity is unknown.
charts/openhab/values.yaml#L116-L141: replace “shared PVC” with “selected PVC” or describe the two supported modes.charts/openhab/README.md#L139-L150: describe each mount as using its resolved PVC, not always the shared PVC.charts/openhab/templates/NOTES.txt#L77-L97: derive the label from the resolved claim mode and omit generated-claim capacity for existing claims.
📍 Affects 3 files
charts/openhab/values.yaml#L116-L141(this comment)charts/openhab/README.md#L139-L150charts/openhab/templates/NOTES.txt#L77-L97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/openhab/values.yaml` around lines 116 - 141, Update
charts/openhab/values.yaml lines 116-141 and charts/openhab/README.md lines
139-150 to describe userdata, conf, and addons as mounting their resolved or
selected PVC, covering both shared and separate existing-claim modes. Update
charts/openhab/templates/NOTES.txt lines 77-97 to derive the displayed label
from the resolved claim mode and omit persistence.claim.size when an existing
claim is used; preserve capacity output for chart-generated claims.
|
@drieks Thank you for the persistence improvements. I reviewed the current head (
The backup subpath initializer itself is present in the current head, and its k3d |
|
Hi @mberlofa, how can I run the make commands?
There are no makefiles in the repository and no description how to execute this commands.. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/openhab/tests/backup_test.yaml (1)
3-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the backup init container’s
subPathmounts.The subpath test checks only
initContainers[0],create-backup-subpaths. Add assertions forinitContainers[1],backup, to verify both mounts include the configuredsubPathandreadOnly: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/openhab/tests/backup_test.yaml` around lines 3 - 5, Extend the tests in backup_test.yaml for initContainers[1] named backup, asserting both volume mounts use the configured subPath and readOnly: true, matching the existing assertions for create-backup-subpaths.
🧹 Nitpick comments (1)
charts/openhab/tests/backup_test.yaml (1)
65-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the backup container subPath mounts.
This test verifies only
create-backup-subpaths. It does not verify that the followingbackupinit container mounts both claims with the configuredsubPathvalues andreadOnly: true.Add assertions for
/openhab/userdataand/openhab/confoninitContainers[1]. This covers the runtime mounts that the initializer prepares.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/openhab/tests/backup_test.yaml` around lines 65 - 89, Add assertions to the test for initContainers[1] (the backup container), verifying its volume mounts use /openhab/userdata and /openhab/conf with the configured shared/openhab/userdata and shared/openhab/conf subPath values and readOnly: true for both claims. Keep the existing create-backup-subpaths assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/openhab/templates/_helpers.tpl`:
- Around line 118-149: Update openhab.validatePersistence to validate shared-PVC
mode: when persistence.claim is enabled, reject differing legacy per-directory
size, storageClass, or accessMode overrides, or require explicit
persistence.claim settings before rendering. In charts/openhab/UPGRADING.md
lines 63-68, require the openHAB workload to be quiesced before copying data and
document prepopulating the chart-managed claim via a pre-provisioned shared
existingClaim before switching mounts.
In `@charts/openhab/templates/NOTES.txt`:
- Line 77: Update the NOTES descriptions for userdata, addons, and backups to
use the resolved claim source rather than only persistence.claim.enabled:
distinguish generated shared claims, shared existing claims, and dedicated
existing claims, and omit configured sizes whenever the PVC already exists. Add
Helm render coverage covering all three claim scenarios and verify the labels
and size suppression for each.
---
Outside diff comments:
In `@charts/openhab/tests/backup_test.yaml`:
- Around line 3-5: Extend the tests in backup_test.yaml for initContainers[1]
named backup, asserting both volume mounts use the configured subPath and
readOnly: true, matching the existing assertions for create-backup-subpaths.
---
Nitpick comments:
In `@charts/openhab/tests/backup_test.yaml`:
- Around line 65-89: Add assertions to the test for initContainers[1] (the
backup container), verifying its volume mounts use /openhab/userdata and
/openhab/conf with the configured shared/openhab/userdata and
shared/openhab/conf subPath values and readOnly: true for both claims. Keep the
existing create-backup-subpaths assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 286c2793-393c-4d9c-8690-ab5f33d41cee
📒 Files selected for processing (15)
charts/openhab/DESIGN.mdcharts/openhab/README.mdcharts/openhab/UPGRADING.mdcharts/openhab/docs/storage.mdcharts/openhab/templates/NOTES.txtcharts/openhab/templates/_helpers.tplcharts/openhab/templates/backup-cronjob.yamlcharts/openhab/templates/pvc.yamlcharts/openhab/templates/statefulset.yamlcharts/openhab/tests/backup_test.yamlcharts/openhab/tests/pvc_test.yamlcharts/openhab/tests/statefulset_test.yamlcharts/openhab/tests/validation_test.yamlcharts/openhab/values.schema.jsoncharts/openhab/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- charts/openhab/README.md
- charts/openhab/values.schema.json
- charts/openhab/values.yaml
- charts/openhab/templates/statefulset.yaml
| {{- define "openhab.validatePersistence" -}} | ||
| {{- $volumes := dict "userdata" .Values.persistence.userdata "conf" .Values.persistence.conf "addons" .Values.persistence.addons }} | ||
| {{- range $name, $volume := $volumes }} | ||
| {{- if and (not $volume.enabled) $volume.existingClaim }} | ||
| {{- fail (printf "persistence.%s must be enabled when existingClaim is configured." $name) }} | ||
| {{- end }} | ||
| {{- if and (hasKey $volume "subPath") (not $volume.existingClaim) $.Values.persistence.claim.enabled }} | ||
| {{- fail (printf "persistence.%s.subPath requires existingClaim when persistence.claim.enabled is true." $name) }} | ||
| {{- end }} | ||
| {{- if and $volume.subPath (or (hasPrefix "/" $volume.subPath) (eq $volume.subPath ".") (eq $volume.subPath "..") (regexMatch "(^|/)\\.\\.(/|$)" $volume.subPath)) }} | ||
| {{- fail (printf "persistence.%s.subPath must be a relative path without traversal segments." $name) }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- range $name, $volume := $volumes }} | ||
| {{- if and $volume.enabled $volume.existingClaim (not $volume.subPath) }} | ||
| {{- range $otherName, $otherVolume := $volumes }} | ||
| {{- if and (ne $name $otherName) $otherVolume.enabled (eq $volume.existingClaim $otherVolume.existingClaim) }} | ||
| {{- fail (printf "existingClaim %q is used by multiple directories; set subPath for every directory that uses the same existing PVC." $volume.existingClaim) }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- range $name, $volume := $volumes }} | ||
| {{- if and $volume.enabled $volume.existingClaim $volume.subPath }} | ||
| {{- range $otherName, $otherVolume := $volumes }} | ||
| {{- if and (ne $name $otherName) $otherVolume.enabled (eq $volume.existingClaim $otherVolume.existingClaim) (eq $volume.subPath $otherVolume.subPath) }} | ||
| {{- fail (printf "existingClaim %q uses subPath %q for multiple directories; each directory must use a distinct subPath." $volume.existingClaim $volume.subPath) }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make shared-PVC migration explicit and safe.
Shared-PVC mode uses only persistence.claim settings. Existing per-directory size, storageClass, and accessMode values can be silently ignored. The migration procedure also permits copying active data and does not provide a safe way to prepopulate the chart-managed claim before the StatefulSet switches to it.
charts/openhab/templates/_helpers.tpl#L118-L149: reject legacy per-directory overrides that differ from supported migration defaults, or require explicit shared-claim settings before shared-PVC mode renders.charts/openhab/UPGRADING.md#L63-L68: require a quiesced openHAB workload before copying data. Document a supported prepopulation path, such as a pre-provisioned sharedexistingClaim, before changing mounts.
📍 Affects 2 files
charts/openhab/templates/_helpers.tpl#L118-L149(this comment)charts/openhab/UPGRADING.md#L63-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/openhab/templates/_helpers.tpl` around lines 118 - 149, Update
openhab.validatePersistence to validate shared-PVC mode: when persistence.claim
is enabled, reject differing legacy per-directory size, storageClass, or
accessMode overrides, or require explicit persistence.claim settings before
rendering. In charts/openhab/UPGRADING.md lines 63-68, require the openHAB
workload to be quiesced before copying data and document prepopulating the
chart-managed claim via a pre-provisioned shared existingClaim before switching
mounts.
|
|
||
| {{- if .Values.persistence.userdata.enabled }} | ||
| userdata ({{ .Values.persistence.userdata.size }}): Runtime state, logs, JSONDB | ||
| userdata ({{ if .Values.persistence.claim.enabled }}shared PVC {{ .Values.persistence.claim.size }}{{ else }}{{ .Values.persistence.userdata.size }} dedicated PVC{{ end }}): Runtime state, logs, JSONDB |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not report configured sizes for existing PVCs.
The persistence.claim.enabled condition distinguishes shared mode from dedicated mode, but it does not distinguish generated PVCs from existing PVCs. As a result, Line 77 can display persistence.claim.size for a shared existing PVC, and Lines 86 and 95 can display per-directory sizes for dedicated existing PVCs. Those values may not describe the actual PVC capacity.
Use the resolved claim source for the label and size. Show a size only when Helm creates the PVC. Use distinct wording for generated shared claims, shared existing claims, and dedicated existing claims. Add render coverage for all three cases.
Also applies to: 86-86, 95-95
🧰 Tools
🪛 LanguageTool
[style] ~77-~77: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sistence.claim.enabled }}shared PVC {{ .Values.persistence.claim.size }}{{ else }}{{ ....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~77-~77: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....persistence.claim.size }}{{ else }}{{ .Values.persistence.userdata.size }} dedicated ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/openhab/templates/NOTES.txt` at line 77, Update the NOTES descriptions
for userdata, addons, and backups to use the resolved claim source rather than
only persistence.claim.enabled: distinguish generated shared claims, shared
existing claims, and dedicated existing claims, and omit configured sizes
whenever the PVC already exists. Add Helm render coverage covering all three
claim scenarios and verify the labels and size suppression for each.
|
@drieks You're right — that command belongs to our maintainer tooling and is not available in this repository. Sorry for the confusion. From the helm dependency build charts/openhab
helm lint --strict charts/openhab
helm template openhab-test charts/openhab
helm unittest charts/openhabPushing your changes will also run the repository CI. We'll handle the internal k3d validation on our side. |
2 participants
Summary
userdata,conf, andaddonsthrough configurablesubPaths and create those directories in an init container.Type Of Change
PR Governance
Resolves #NNNorRelated to #NNN)enhancementandtype:featureare appliedChecklist
mainmainversioninChart.yamlmanuallyREADME.mdif a new chart was added or public chart metadata changedvalues.schema.jsonfor any values changesUpstream Verification
appVersioninChart.yamlmatches the real upstream releasevalues.yamlcorresponds to a published, stable tagSite Sync (GR-007)
site/repositoryLocal Validation
kubectl config current-contextbefore local installs/upgrades/uninstallshelm lint charts/openhab --strictpassedhelm unittest charts/openhabpassedci/*.yamlscenarios rendered successfullyk3dcluster when requiredNotes
lost+founddirectory and openhab does not handle such PVCs as empty. on first start no template data is copied into such PVCs.helm lint charts/openhab --strictand allcharts/openhab/ci/*.yamlrenders passed.helm unittest charts/openhabcurrently fails immediately withEOFinbackup_test.yamlusing Helm 4.2.2 and helm-unittest 0.6.3; no tests were executed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation