Add Jenny's Helm chart #5

claudusd · 2024-05-15T12:38:11Z

This PR add the Helm chart from Jenny. A Django base app.

wilbrdt

Nice work, really good model!
A few general remarks:

some files are missing an EOF newline
labels and annotations seem a bit inconsistent, which makes them harder to use

wilbrdt · 2024-05-15T14:47:47Z

jenny/templates/deployment.yaml

+            {{- include "jenny.envs" . | nindent 12 }}
+            - name: "UWSGI_PORT"
+              value: "{{ .Values.django.port }}"
+          {{- if .Values.persistence.enabled }}


I would align this with its closing {{- end }}, for readability. WDYT?

I remove this part.

wilbrdt · 2024-05-15T14:55:06Z

jenny/templates/deployment.yaml

+              port: {{ .Values.django.port }}
+              httpHeaders:
+                - name: "Host"
+                  value: "{{ .Values.ingress.host}}"


I think ingress should have a default host value

wilbrdt · 2024-05-15T15:05:16Z

jenny/templates/_helpers.tpl

+service: app
+{{- end }}
+
+{{- define "jenny.envs" -}}


nit:

Suggested change

{{- define "jenny.envs" -}}

{{/*

Environment variables

*/}}

{{- define "jenny.envs" -}}

wilbrdt · 2024-05-15T15:07:46Z

jenny/templates/_helpers.tpl

+{{- end }}
+{{- end }}
+
+{{- define "django.imagePullSecrets" -}}


nit:

Suggested change

{{- define "django.imagePullSecrets" -}}

{{/*

ImagePullSecrets

*/}}

{{- define "django.imagePullSecrets" -}}

wilbrdt · 2024-05-15T15:10:45Z

jenny/templates/jobs.yaml

🥇 nice work, thanks! 🙏

wilbrdt · 2024-05-15T15:29:42Z

jenny/templates/jobs.yaml

+    {{- if $.Values.commonLabels }}
+    {{ toYaml $.Values.commonLabels | nindent 4 }}
+    {{- end }}
+    app.kubernetes.io/component: jenny


What is this label used for?
If useful, could we use a helper definition to specify it?

wilbrdt · 2024-05-15T15:49:20Z

jenny/values.yaml

+                  - "jenny-app"
+          topologyKey: kubernetes.io/hostname
+
+autoscaling:


Should we provide a HorizontalPodAutoscaler?

wilbrdt · 2024-05-15T16:52:49Z

jenny/templates/_helpers.tpl

+  value: "{{ .Values.django.db.host }}"
+- name: "DB_PORT"
+  value: "{{ .Values.django.db.port }}"
+{{- range $key, $val := .Values.env.secret }}


When linting this chart, a warning seems to be raised because of this line. It is resolved when adding an empty default value as so:

env: secret: []

wilbrdt · 2024-05-16T10:43:54Z

jenny/values.yaml

+podAnnotations: {}
+
+service:
+  port: 8080


This service port isn't used by any template, should it be removed?

wilbrdt · 2024-05-16T12:35:53Z

jenny/templates/configmap.yaml

+metadata:
+  name: "{{ template "jenny.fullname" . }}-nginx"


We're missing common labels and annotations here. Is that OK?

Rework Warren Helm chart based on the work done on Jenny Helm chart in PR openfun/charts#5 Changes done in this commit: - Jobs for database migration or static collection are abstracted into a single job template - Env variables are now processed in the helper template - Removed unused HorizontalPodAutoscaler - Add startup probes for migration checks - Migration jobs are now executed post helm installation/upgrade - Removed unnecessary security context variables - Add a nginx container alongside warren-app to serve static files - Rework postgresql values to have a functional Helm chart on a local cluster - Update Helm chart README.md

wilbrdt · 2024-05-17T09:20:01Z

jenny/templates/deployment.yaml

+            items:
+              - key: config
+                path: nginx.conf
+            name: "{{ .Chart.Name }}-nginx"


Suggested change

name: "{{ .Chart.Name }}-nginx"

name: "{{ template "jenny.fullname" . }}-nginx"

wilbrdt · 2024-05-17T09:31:07Z

jenny/templates/deployment.yaml

+          command:
+            - "python"
+            - "manage.py"
+            - "collectstatic"
+            - "--no-input"


It seems that the Django SECRET_KEY is needed to execute this command.
We should probably add the env variables to the initContainer. WDYT?

wilbrdt · 2024-05-21T15:20:55Z

jenny/templates/ingress.yaml

+    {{- end }}
+spec:
+  ingressClassName: {{ .Values.ingress.class_name }}
+  {{- if $.Values.ingress.tls.enable }}


Suggested change

{{- if $.Values.ingress.tls.enable }}

{{- if $.Values.ingress.tls.enabled }}

jenny/templates/ingress.yaml

Ingore the tgz build with the command helm package.

This annotations need to be add to use the cert-manager for certificate request.

The host is not mandatory in the ingress definition but for an HTTP app it's a best pratice to use an host instead of an IP. And the host is mandatory for HTTPS.

Add the possibility to enable the HTTPS and to set the certificate name.

The Jenny image is build to ensure the user cannot write in the app dir. Allow to change the running user is not required.

Django required to collect the static and service with a HTTP server. We add an Nginx container in the pods and add an init container to collect this statics. And use an ephemeral volume for static instead of a persistent one.

Fix when create a list of job in the same template and ensure to run the job after the install to avoid missing resource.

This allow from the values to add environment variable from secret to configure Jenny's app.

Ensure to ingore previous helm package in the next helm package.

For the Django app we use the hearbeat add by the lib dockerflow for the liveness and readiness probes Use Nginx status page for liveness probe. And check migration for the startup probe.

Rework Warren Helm chart based on the work done on Jenny Helm chart in PR openfun/charts#5 Changes done in this commit: - Jobs for database migration or static collection are abstracted into a single job template - Env variables are now processed in the helper template - Removed unused HorizontalPodAutoscaler - Add startup probes for migration checks - Migration jobs are now executed post helm installation/upgrade - Removed unnecessary security context variables - Add a nginx container alongside warren-app to serve static files - Rework postgresql values to have a functional Helm chart on a local cluster - Update Helm chart README.md

wilbrdt · 2024-05-22T13:44:55Z

jenny/templates/configmap.yaml

+            allow 127.0.0.1;
+            deny all;


Suggested change

allow 127.0.0.1;

deny all;

Rework Warren Helm chart based on the work done on Jenny Helm chart in PR openfun/charts#5 Changes done in this commit: - Jobs for database migration or static collection are abstracted into a single job template - Env variables are now processed in the helper template - Removed unused HorizontalPodAutoscaler - Add startup probes for migration checks - Migration jobs are now executed post helm installation/upgrade - Removed unnecessary security context variables - Add a nginx container alongside warren-app to serve static files - Rework postgresql values to have a functional Helm chart on a local cluster - Update Helm chart README.md

claudusd requested a review from wilbrdt May 15, 2024 12:38

claudusd changed the title ~~Improve jenny chart~~ Add Jenny's Helm chart May 15, 2024

wilbrdt reviewed May 16, 2024

View reviewed changes

wilbrdt mentioned this pull request May 16, 2024

🔧(helm) rework chart based on jenny helm chart openfun/warren#252

Open

9 tasks

wilbrdt reviewed May 21, 2024

View reviewed changes

jenny/templates/ingress.yaml Show resolved Hide resolved

claudusd added 10 commits May 22, 2024 11:21

🙈(ignore) add gitignore file

851327c

Ingore the tgz build with the command helm package.

🔧(jenny) add anotations on the ingress

14586c4

This annotations need to be add to use the cert-manager for certificate request.

🔒️(jenny) required host for the ingress

a98a438

The host is not mandatory in the ingress definition but for an HTTP app it's a best pratice to use an host instead of an IP. And the host is mandatory for HTTPS.

✨(jenny) add tls support for the ingress

ef2351d

Add the possibility to enable the HTTPS and to set the certificate name.

🔧(jenny) remove securityContext

5d86a8d

The Jenny image is build to ensure the user cannot write in the app dir. Allow to change the running user is not required.

🔧(jenny) add nginx in the pod

e09feb0

Django required to collect the static and service with a HTTP server. We add an Nginx container in the pods and add an init container to collect this statics. And use an ephemeral volume for static instead of a persistent one.

🐛(jenny) fix job creation

ea6fbec

Fix when create a list of job in the same template and ensure to run the job after the install to avoid missing resource.

✨(jenny) allow to add env from secret

f831a2b

This allow from the values to add environment variable from secret to configure Jenny's app.

🙈(jenny) add helmignore

75a4221

Ensure to ingore previous helm package in the next helm package.

🔧(jenny) use proper check for kube probes

e9a004b

For the Django app we use the hearbeat add by the lib dockerflow for the liveness and readiness probes Use Nginx status page for liveness probe. And check migration for the startup probe.

claudusd force-pushed the jenny/improve branch from 97e54d0 to e9a004b Compare May 22, 2024 09:22

claudusd added 4 commits May 22, 2024 13:37

fixup! 🔧(jenny) add nginx in the pod

52744cf

fixup! ✨(jenny) allow to add env from secret

48b7052

fixup! ✨(jenny) add tls support for the ingress

144a430

fixup! 🔧(jenny) use proper check for kube probes

3825e0a

wilbrdt reviewed Jun 6, 2024

View reviewed changes

jenny/templates/configmap.yaml Outdated

Comment on lines 48 to 49

allow 127.0.0.1;

deny all;

Copy link

wilbrdt May 22, 2024

Choose a reason for hiding this comment

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

Suggested change

allow 127.0.0.1;

deny all;

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Jenny's Helm chart #5

Add Jenny's Helm chart #5

claudusd commented May 15, 2024

wilbrdt left a comment

wilbrdt May 15, 2024

claudusd May 23, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 15, 2024

wilbrdt May 16, 2024

wilbrdt May 16, 2024

wilbrdt May 17, 2024

wilbrdt May 17, 2024

wilbrdt May 21, 2024

wilbrdt May 22, 2024

	name: "{{ .Chart.Name }}-nginx"
	name: "{{ template "jenny.fullname" . }}-nginx"

	{{- if $.Values.ingress.tls.enable }}
	{{- if $.Values.ingress.tls.enabled }}

Add Jenny's Helm chart #5

Are you sure you want to change the base?

Add Jenny's Helm chart #5

Conversation

claudusd commented May 15, 2024

wilbrdt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment