add geoserver helm chart #17

tremolo · 2018-09-04T10:30:04Z

No description provided.

MrPink · 2018-09-04T14:14:04Z

src/geoserver/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+appVersion: "1.0"


is this the GeoServer version?

no geoserver version is specified here:
https://github.com/eHealthAfrica/solution-deployment/blob/gcp_deployment/Kaduna/prod/geoserver/geoserver.yaml#L3

This was generated by helm

MrPink · 2018-09-04T14:14:46Z

src/geoserver/setup/masterpassword.xml

+set:
+curl -v -u admin:geoserver -XPUT -H "Content-type: text/xml" -d @masterpassword.xml masterpassword.xml \
+https://geoserver-kaduna.ehealthafrica.org/geoserver/rest/security/masterpw.xml 
+-->


what is this file do?

oops not supposed to be here. it was a the way how to change the default admin and masterpassword

src/geoserver/templates/deployment.yaml

MrPink · 2018-09-04T14:36:35Z

src/geoserver/values.yaml

@@ -0,0 +1,68 @@
+# Default values for geoserver.


can you strip out the ehealth and nginx specific info please, I'll put that in the project override

MrPink · 2018-09-04T14:36:50Z

src/geoserver/values.yaml

+
+
+
+database:


you can remove this too

MrPink · 2018-09-04T14:37:18Z

src/geoserver/values.yaml

+  instance:
+
+
+resources: {}


and everything below here

MrPink · 2018-09-04T14:42:06Z

src/geoserver/templates/deployment.yaml

+            readOnly: true
+      {{ end }}
+      - name: {{ .Chart.Name }}
+        image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"


can you please do something like image: "{{ .Values.geoserver.repository }}:{{ .Values.geoserver.tag | default .Chart.AppVersion }}" (making sure the AppVersion is corect

tooxie · 2018-09-05T07:23:54Z

src/geoserver/.helmignore

+# Patterns to ignore when building packages.
+# This supports shell glob matching, relative path matching, and
+# negation (prefixed with !). Only one pattern per line.
+.DS_Store


I would remove all these extensions and leave only the ones that apply to our project.

this was generated by helm, i removed some of the unused stuff..

tooxie · 2018-09-05T07:26:02Z

index.yaml

+    digest: 49fbc81f02a96a449dba13e701d819cdcec952305660d5080167f3d5cd8aec18
+    name: kernel
+    urls:
+    - https://ehealthafrica.github.io/helm-charts/charts/kernel-0.9.0.tgz


This seems to be adding charts to the index that don't exist. Please remove all the charts that are not strictly the geoserver, if we have to revert this commit we don't want to discard unrelated charts as well.

this file is regenerated every time we publish, so it is not required to track this.

tooxie · 2018-09-05T07:26:35Z

src/geoserver/Chart.yaml

+appVersion: "2.13.0"
+description: A Helm chart for Kubernetes
+name: geoserver
+version: 0.1.0


Is there a reason why we don't start at 0.0.1?

switched to 0.0.1 for consistency with the other modules

tooxie · 2018-09-05T07:27:09Z

src/geoserver/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+appVersion: "2.13.0"
+description: A Helm chart for Kubernetes


Please update the description, this will be visible for users who want to install the chart.

tooxie · 2018-09-05T07:27:49Z

src/geoserver/templates/NOTES.txt

+{{- else if contains "ClusterIP" .Values.service.type }}
+  export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ template "geoserver.name" . }},release={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}")
+  echo "Visit http://127.0.0.1:8080 to use your application"
+  kubectl port-forward $POD_NAME 8080:80


Are these instructions accurate? Can I access the app if I follow these steps?

update the target port for port-forward

tooxie · 2018-09-05T07:36:17Z

src/geoserver/templates/ingress.yaml

@@ -0,0 +1,34 @@
+{{- $serviceName := include "geoserver.name" . -}}
+{{- $servicePort := .Values.service.externalPort -}}


Why add the indirection of a variable for a simple value? If servicePort is a better name than externalPort then we should change the value name instead. And why is it only renamed here and not in service.yaml?

if this is used directly the templating fails, because {{ .Values.service.externalPort }}can not be resolved inside the loop

tooxie · 2018-09-05T07:36:52Z

src/geoserver/templates/init-config-map.yaml

+    # chmod -R 775 /media
+    # chown -R 1000:101 /media
+
+{{ else }}


{{- else }}

removing whitespace here does not make things better but instead can give problems

tooxie · 2018-09-05T07:39:17Z

src/geoserver/templates/volume_claim.yaml

+kind: PersistentVolumeClaim
+apiVersion: v1
+metadata:
+  name: {{ .Chart.Name }}


In the other charts we added the suffix -pvc to the claims, so that when in the logs we see the {{ .Chart.Name }} we know immediately what we are talking about.

tooxie · 2018-09-05T07:41:04Z

src/geoserver/templates/deployment.yaml

+      containers:
+      {{ if .Values.provider.gcp }}
+      - name: cloudsql-proxy
+        image: gcr.io/cloudsql-docker/gce-proxy:1.11


It's useful to have a pullPolicy for every image, in case we need to force a re-download.

this is unlikely to happen for a 3rd party image, the default of IfNotPresent should be ok

tooxie · 2018-09-05T07:43:00Z

src/geoserver/templates/ingress.yaml

@@ -0,0 +1,34 @@
+{{- $serviceName := include "geoserver.name" . -}}


What's the difference between include and template? Why do we have a variable for one but not for the other?

tooxie · 2018-09-05T14:22:24Z

src/geoserver/templates/deployment.yaml

+        imagePullPolicy: {{ .Values.image.pullPolicy }}
+        env:
+        - name: DEBUG
+          value:  {{ quote .Values.debug }}


This is inconsistent with how the other charts handle the DEBUG value. Is this because of an app constraint?

adapted to other charts

tooxie · 2018-09-05T14:25:36Z

src/geoserver/values.yaml

+  data_mount: /opt/geoserver/data_dir
+
+geoserver:
+  repository: kartoza/geoserver


IIRC we call that image in the other charts.

rearranged structure to match other charts

tooxie · 2018-09-05T14:28:13Z

src/geoserver/values.yaml

+  tag: 2.13.0
+
+image:
+  pullPolicy: IfNotPresent


Why is this under image while the image we apply it to under geoserver? I would move it to geoserver.

rearranged structure to match other charts

tooxie · 2018-09-05T14:28:44Z

src/geoserver/values.yaml

+# Declare variables to be passed into your templates.
+
+replicaCount: 1
+application:


We call this app in the other charts.

tooxie · 2018-09-05T14:30:43Z

src/geoserver/templates/deployment.yaml

+          periodSeconds: 10
+          httpGet:
+            path: /
+            port: 8080


All these port numbers should use the values defined in the values file.

tooxie · 2018-09-05T14:32:14Z

src/geoserver/values.yaml

+  type: ClusterIP
+  externalPort: 8080
+  internalPort: 8080
+  port: 8080


Are these 4 values going to change? Do we want to make them configurable? We removed them from the other charts.

port has been unified and updated in all referenced locations

femilofin · 2018-10-07T20:05:51Z

src/geoserver/templates/service.yaml

+  type: {{ .Values.service.type }}
+  ports:
+    - port: {{ .Values.service.port }}
+      targetPort: {{ .Values.service.port }}


should be {{ .Values.app.port }}

add geoserver helm chart

4997700

tremolo requested a review from MrPink September 4, 2018 10:30

MrPink requested a review from tooxie September 4, 2018 14:13

MrPink suggested changes Sep 4, 2018

View reviewed changes

tremolo added 2 commits September 4, 2018 17:09

apply cleanup for PR

c5caaa0

apply more cleanup for PR

674f27a

MrPink previously approved these changes Sep 4, 2018

View reviewed changes

package charts

0a6b84f

tremolo dismissed MrPink’s stale review via 0a6b84f September 4, 2018 15:51

tooxie suggested changes Sep 5, 2018

View reviewed changes

MrPink and others added 4 commits September 11, 2018 20:48

chore: clean up

e19ccef

chore: we need a replica of 1

cf532ce

apply cleanup for geoserver chart

fbb5aa0

chore: default port

d3497c4

femilofin reviewed Oct 7, 2018

View reviewed changes

		@@ -0,0 +1,34 @@
		{{- $serviceName := include "geoserver.name" . -}}
		{{- $servicePort := .Values.service.externalPort -}}




		database:

		instance:


		resources: {}

add geoserver helm chart #17

Are you sure you want to change the base?

add geoserver helm chart #17

Conversation

tremolo commented Sep 4, 2018

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

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

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