[stable/drupal] Allow non-bitnami image Drupal data and Apache configuration volume paths #1232
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @scottrigby. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
scottrigby
changed the title
|
@scottrigby it's not required, but it persists the apache configuration in a volume in case you need to back it up, or modify it |
stable/drupal/README.md
Outdated
| @@ -88,9 +90,17 @@ $ helm install --name my-release -f values.yaml stable/drupal | |||
|
|
|||
| > **Tip**: You can use the default [values.yaml](values.yaml) | |||
|
|
|||
| ## Persistence | |||
| ## Image | |||
There was a problem hiding this comment.
This section seems redundant to me, as it's in the configuration table and there isn't really anything further to explain about this value?
There was a problem hiding this comment.
You're right. I was trying to make a header that would encompass subheaders for both the existing Persistence header and this new volume mounts header - and thought Image might make sense. But maybe you're right. Let me remove that extra image header hierarchy, and see if it seems less redundant?
There was a problem hiding this comment.
@prydonius OK, how does that read now?
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
scottrigby
force-pushed
the
drupal-volume-mount-paths
branch
from
3fdf1c4 to
b389233
Compare
|
@prydonius rebased, and correctly bumped version (note the bump is the same as #1340 because I'm not sure which will be merged first. Once one is, I can bump the other PR PATCH version again. Or you can if you prefer) |
scottrigby
force-pushed
the
drupal-volume-mount-paths
branch
from
b389233 to
c0b35f4
Compare
|
@prydonius OK rebased again, after #1340 was merged. This time I bumped the MINOR version (I should have bumped this PR MINOR version in the first place). |
…Path is set to empty string
|
@prydonius I also added an additional conditional in f131108. This was important for an image I tested that doesn't store anything in the apache conf directory, and so didn't need to be mounted. Also, it didn't build apache configs at runtime, so the dir was always overwritten by an empty object. This wasn't an issue with the bitnami-docker-drupal image because it has matching logic, but for other images this was difficult. I'm open to other ways of doing this though if you have suggestions? |
|
@prydonius The last commit f6cc19 fixes an issue with this patch on Tectonic/AWS. GCP was fine. |
|
/ok-to-test |
prydonius
added
code reviewed
lgtm
…uration volume paths (helm#1232) * Specify Drupal data and Apache configuration volume mount paths * Simplify README changes * Bump stable/drupal minor version for new feature * Allow not mounting Apache configs, if value volumeMounts.apache.mountPath is set to empty string * Don't create the Apache PVC if mountPath is null
…uration volume paths (helm#1232) * Specify Drupal data and Apache configuration volume mount paths * Simplify README changes * Bump stable/drupal minor version for new feature * Allow not mounting Apache configs, if value volumeMounts.apache.mountPath is set to empty string * Don't create the Apache PVC if mountPath is null
Follow up to #1231.
@prydonius As an aside, I'm unclear why we mount the Apache config directory? I'm sure I'm missing something obvious about this.