Nginx updates #20
Nginx updates #20
Conversation
Fixed missing params from values file and added resources
|
Added "default" chart which is more configurable and now Nginx is deployable. The version is changed to 1.0 as this chart will probably break the current deployment. |
There was a problem hiding this comment.
@erikburgess There's a whole bunch of deletions of what might be useful stuff. Can you explain?
src/nginx/values.yaml
Outdated
| @@ -21,4 +57,4 @@ imagePullPolicy: IfNotPresent | |||
| # fastcgi_index index.php; | |||
| # include fastcgi.conf; | |||
| # } | |||
| # } | |||
| # } | |||
| maintainers: | ||
| - email: [email protected] | ||
| name: Bitnami | ||
| apiVersion: v1 |
There was a problem hiding this comment.
Why is most of this file removed?
There was a problem hiding this comment.
IMO this is a new chart, it is created from a "helm create" and supports the same features.
| @@ -1,99 +0,0 @@ | |||
| # NGINX | |||
There was a problem hiding this comment.
Why is this readme deleted? Every chart needs a readme.
| template: | ||
| metadata: | ||
| labels: | ||
| app: {{ template "fullname" . }} | ||
| chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" | ||
| release: "{{ .Release.Name }}" |
There was a problem hiding this comment.
I think we're losing infomration here. Labels that we rely upon to filter for the helm release and heritage
There was a problem hiding this comment.
These still exist, they are replaced with the current values of nginx.XXXXXXX. The chart label is missing because helm does not create it anymore.
src/nginx/templates/deployment.yaml
Outdated
| livenessProbe: | ||
| httpGet: | ||
| path: / | ||
| port: http | ||
| initialDelaySeconds: 30 |
|
@dcwangmit01 I didn't realize this chart was taken from an upstream project, but the changes are drastic enough that it seemed seemed like a new chart. |
|
Please update so that the deleted items don't cause regressions. @rustycl0ck Needs to review this changes to ensure they don't break anything. |
|
@erikburgess
Couldn't tag them in the PR because these lines weren't actually changed. After updating these variables, I was able to successfully apply this chart. |
|
Still awaiting the changes that @rustycl0ck requested. |
|
@erikburgess If you're breaking something, please include the PR to fix that which will be broken. After we see that PR, we can merge this one and that one sequentially. Please work with @rustycl0ck |
Added service type to configuration to prevent deployment issues.
Added the ability to configure resources to the deployment.