-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
@k8s-bot ok to test |
/test |
Can you add a default PW for the mysql like in the MySQL chart? This will allow the chart to install with the default values and make CI go through. |
BTW awesome chart!!!!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a default PW (hopefully random) for the MySQL root PW.
@viglesiasce |
/retest |
…ependency removed by adding the httpd config files to the chart
@viglesiasce I'd like to contribute a 'nextcloud' chart which uses the lamp chart as a dependency. How long will it take to accept this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at our best practices, especially regarding standard labels. Make sure all resources get our standard labels.
https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md
Note that we've started namespacing template (see #1785). Please apply this to your chart.
Please update the PR with latest master.
apiVersion: v1 | ||
description: Modular and transparent LAMP stack chart supporting PHP-FPM, Release Cloning, LoadBalancer, Ingress, SSL and lots more! | ||
name: lamp | ||
version: 0.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add appVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave this off since there is a conglomeration of app versions.
stable/lamp/README.md
Outdated
|
||
## Prerequisites | ||
|
||
- Kubernetes 1.4+ with Beta APIs enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.7+
metadata: | ||
labels: | ||
app: {{ template "fullname" . }} | ||
annotations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use initContainers
field on the PodSpec instead of annotation.
stable/lamp/templates/ingress.yaml
Outdated
@@ -0,0 +1,126 @@ | |||
{{- if .Values.ingress.enabled -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One file per resource, please.
stable/lamp/templates/ingress.yaml
Outdated
kind: Ingress | ||
metadata: | ||
name: {{ template "fullname" . }}-www | ||
annotations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of wrapping various annotations in if blocks, I'd suggest you add the annotations to the configuration. This would also be more flexible.
stable/lamp/templates/pvc.yaml
Outdated
heritage: "{{ .Release.Service }}" | ||
{{- if .Values.persistence.storageClass }} | ||
annotations: | ||
volume.beta.kubernetes.io/storage-class: {{ .Values.persistence.storageClass | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer use annotation for storage class. We've developed a pattern for persistence (see #1869). Please follow this approach.
stable/lamp/templates/service.yaml
Outdated
{{- if and .Values.sftp.enabled .Values.ingress.enabled }} | ||
|
||
--- | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One file per resource, please.
stable/lamp/values.yaml
Outdated
#repository: php | ||
#tag: fpm | ||
pullPolicy: Always | ||
fpm_enabled: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use camel case instead of underscores. Apply everywhere.
Marking this as stale. Please update within one week. |
@unguiculus
|
metadata: | ||
name: {{ template "lamp.fullname" . }}-httpd | ||
labels: | ||
app: {{ template "lamp.fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be app: {{ template "lamp.name" . }}
. Please update everywhere.
protocol: TCP | ||
name: sftp | ||
selector: | ||
app: {{ template "lamp.fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add release
label to selector.
stable/lamp/templates/service.yaml
Outdated
name: phpmyadmin | ||
{{ end }} | ||
selector: | ||
app: {{ template "lamp.fullname" . }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add release
label to selector.
@unguiculus updated |
@viglesiasce Could you have another look, please? |
apiVersion: v1 | ||
description: Modular and transparent LAMP stack chart supporting PHP-FPM, Release Cloning, LoadBalancer, Ingress, SSL and lots more! | ||
name: lamp | ||
version: 0.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave this off since there is a conglomeration of app versions.
stable/lamp/values.yaml
Outdated
## `ingress.kubernetes.io/proxy-body-size | ||
annotations: | ||
ingress.kubernetes.io/proxy-body-size: "50m" | ||
kubernetes.io/ingress.class: nginx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should leave these commented so that you can use the default ingress in the cluster, unless the proxy-body-size and NGINX is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, nginx ingress controller is not mandatory although some features such as ssl are only working with nginx controller. I've commented that part out and added explanations regarding nginx controller requirements to the values which need the controller.
/ok-to-test |
awesome in that case: /lgtm @unguiculus all set on my end, merge when you are ready |
See lamp/README.md for details of the chart. Let me know if you have any suggestions to complete the pull request.