Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/drupal] Use storageClassName for drupal. #1989

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

gyliu513
Copy link
Contributor

@gyliu513 gyliu513 commented Sep 5, 2017

Fixed drupal for #1869

/cc @prydonius

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2017
Copy link
Member

@prydonius prydonius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait for mariadb

cc @scottrigby r.e. hostmount

@@ -25,4 +17,13 @@ spec:
resources:
requests:
storage: {{ .Values.persistence.drupal.size | quote }}
{{- if (not .Values.persistence.drupal.hostPath) }}
{{- if .Values.persistence.drupal.storageClass }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be

{{- if and .Values.persistence.drupal.storageClass (not .Values.persistence.drupal.hostPath) -}}

you might need to wrap the and in parens too, be sure to test it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks @prydonius

@gyliu513
Copy link
Contributor Author

gyliu513 commented Sep 6, 2017

@prydonius mariadb is here #2004

@scottrigby
Copy link
Member

Reviewing this re hostPath concerns… 👀 ⚙️

@gyliu513
Copy link
Contributor Author

gyliu513 commented Sep 8, 2017

@scottrigby this just make sure that we do not need hostPath when using storageClass.

@scottrigby
Copy link
Member

@gyliu513 This looks correct, just have not yet been able to test yet. Will do this weekend.

@scottrigby
Copy link
Member

scottrigby commented Sep 11, 2017

@gyliu513 I finally got to check this and ensured hostPath still works as expected LGTM 👍 CC @prydonius

@gyliu513
Copy link
Contributor Author

Thanks @scottrigby for the test, @prydonius , can we get this merged?

Copy link
Member

@prydonius prydonius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2017
digest: sha256:e1af13ac4ac21f67582006f12d2b4eb78a1a2a59b34338fac850f2bec0b08b41
generated: 2017-08-09T22:52:48.218901909-04:00
version: 1.0.5
digest: sha256:08de643d4a3b2886a76b7f5da8ea7db20b7a69b8de1d04594862adfe4735993c
Copy link
Contributor Author

@gyliu513 gyliu513 Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prydonius thanks for the update, can you please show me how does those sha256 generated/how does the timestamp generated, so that I will not forget update here next time. Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so to bump a version you need to update requirements.yaml to the version to bump to. Then you run helm dep update ./path/to/chart and this will fetch the new version, update requirements.lock and populate the sha.

version: 0.7.0
digest: sha256:e1af13ac4ac21f67582006f12d2b4eb78a1a2a59b34338fac850f2bec0b08b41
generated: 2017-08-09T22:52:48.218901909-04:00
version: 1.0.5
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the version here is 1.0.5?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just grabbing the latest version of mariadb which also includes the storageClass update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see, just forget to update my local code..

Copy link
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to leave an actual GitHub "review". LGTM

Copy link
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after rebase 💯

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants