Add extraVolumes and extraVolumeMounts Helm values for mounting files into the operator pod - #65
Conversation
… into the operator pod
There was a problem hiding this comment.
Sorry but I have to close this, I told you in PR #60 that we will do this in a follow-up PR.
Your version actually uses the old workaround for Helm chart values.yaml, I changed that in PR #62.
I opened #62 two hours before this PR, for the same gap, and it carries wider test coverage, and it also moves imagePullSecrets onto the same new pattern we used instead of the workaround you carried on.
I also decided against using extraVolumes and extraVolumeMounts, which matches the convention of Bitnami charts.
The reason is that extra implies that the Deployment has already a volume on it, which is not the case with our operator.
So volumes and volumeMounts match the rest of this chart, where imagePullSecrets and affinity use the raw Kubernetes names.
| value: | ||
| - null | ||
| paths: | ||
| - (kind == Deployment).spec.template.spec.containers.(name == postgresql-operator).volumeMounts |
There was a problem hiding this comment.
The container name is hardcoded here. We should use ${quarkus.kubernetes.name} instead.
Five other paths in this file already do, including line 86 and lines 99 to 109.
A change to quarkus.kubernetes.name breaks this path silently. quarkus-helm finds no node, processValueReference does nothing, and the value never reaches the template. No build error, no test failure, just a chart that ignores extraVolumeMounts.
| # when the value path uses a container name filter like `containers.(name == ...)`. | ||
| # This merge file corrects the default value type. | ||
| app: | ||
| extraVolumeMounts: [] |
There was a problem hiding this comment.
extraVolumes needs the same override.
This file corrects extraVolumeMounts only. I built your branch and read the generated chart:
app:
extraVolumeMounts: []
extraVolumes:
- {}
imagePullSecrets:
- {}So helm show values presents extraVolumes as - {}. A user who copies that block and appends a volume name ships an invalid first entry. The guard expression keeps the default install correct, so nothing breaks until a user edits the value. That is the worst moment for it to break.
|
|
||
| Objects.requireNonNull(appValues, "appValues should not be null"); | ||
| assertThat(appValues.get("image")).isNotNull(); | ||
| assertThat(appValues).containsKey("extraVolumes"); |
There was a problem hiding this comment.
containsKey passes on the broken default. extraVolumes is [{}] in the built chart, and this assertion still passes.
assertThat(appValues.get("extraVolumes")).isEqualTo(List.of());Two further gaps in this test:
- Nothing asserts
type: arrayand thedescriptioninvalues.schema.json. A missingtypefalls back tostring, and thenhelm installrejects a list. - Nothing renders the chart with a real volume.
helm templateneeds no cluster, and it validates the values against the schema. A wrongnindentproduces invalid YAML for every user who sets the value, and no current test sees that.
| .satisfies(spec -> { | ||
| assertThat(spec.getTemplate().getSpec().getImagePullSecrets()).isEmpty(); | ||
| assertThat(spec.getTemplate().getSpec().getVolumes()).isEmpty(); | ||
| assertThat(spec.getTemplate().getSpec().getContainers().getFirst().getVolumeMounts()).isEmpty(); |
There was a problem hiding this comment.
getFirst() hides the risk that the named container introduces.
kubernetes.yml now names a container. If that name ever stops matching quarkus.kubernetes.name, the Pod gets two containers, and getFirst() may still return the right one. A fix would look like this:
assertThat(spec.getTemplate().getSpec().getContainers())
.singleElement()
.satisfies(container -> {
assertThat(container.getName()).isEqualTo("postgresql-operator");
assertThat(container.getVolumeMounts()).isEmpty();
});| > **Note:** The volume source can be any type that provides a file. | ||
|
|
||
| > **Note:** The Helm chart does not support extra volumes yet. | ||
| > **Tip:** When using the Helm chart, configure volumes via `extraVolumes` and `extraVolumeMounts` values: |
There was a problem hiding this comment.
Missing Secrets Store CSI example.
|
Duplicate of #62 |
Summary
extraVolumesandextraVolumeMountsHelm values for mounting files into the operator podadminSecretFileRefusage without patching the deploymentTest plan
./gradlew :operator:test --tests "it.aboutbits.postgresql.helm.HelmTest"