Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make helm template formatting follow best practices #1174

Merged

Conversation

consideRatio
Copy link
Member

This PR is free from any changes to the logic or assumptions on the configuration, it only only adjusts formatting.

@consideRatio consideRatio force-pushed the pr/modernize-template-formatting branch from dcb2e65 to 0f717f2 Compare October 29, 2020 11:50
@minrk
Copy link
Member

minrk commented Oct 29, 2020

I ran helm template ./binderhub -f minikube-binder.yaml and compared before/after this PR. to double-check that the results are as-intended:

--- before.yaml	2020-10-29 13:45:19.000000000 +0100
+++ after.yaml	2020-10-29 13:45:32.000000000 +0100
@@ -1104,6 +1104,9 @@
 metadata:
   name: binder-config
 data:
+  # FIXME: Stop splitting up parts into configmap/secret as it adds complexity,
+  #        instead only use a secret. For the equivalent PR in z2jh, see:
+  #        https://github.com/jupyterhub/zero-to-jupyterhub-k8s/pull/1682
   values.yaml: |
     config:
       BinderHub:
@@ -1483,7 +1486,6 @@
   labels: {}
 spec:
   type: NodePort
-  
   selector:
     app: binder
     name: binder
@@ -1548,7 +1550,7 @@
         - name: IMAGE_GC_DELAY
           value: "5"
         - name: IMAGE_GC_THRESHOLD_TYPE
-          value: relative
+          value: "relative"
         - name: IMAGE_GC_THRESHOLD_HIGH
           value: "80"
         - name: IMAGE_GC_THRESHOLD_LOW
@@ -1852,13 +1854,11 @@
         component: binder
         release: RELEASE-NAME
         heritage: Helm
-        
       annotations:
         # This lets us autorestart when the configmap changes!
-        checksum/config-map: ae0a43fd9e9d31da75751fbe6db89e48ba413df226cce43f853e55258ff9533d
+        checksum/config-map: 1c586d1a2901830eaccb36194aa1451e3eaba06538202254afb0e2d00e0b1141
         checksum/secret: 958722d545be64139fadb4d84570157e6d4d430874e91e4a663c8dc880fbf84e
     spec:
-      
       nodeSelector: {}
       serviceAccountName: binderhub
       volumes:
@@ -1871,7 +1871,6 @@
       - name: docker-socket
         hostPath:
           path: /var/run/docker.sock
-      
       containers:
       - name: binder
         image: jupyterhub/k8s-binderhub:0.2.0-n246.h8c5b0c9
@@ -1885,11 +1884,10 @@
             name: secret-config
           - mountPath: /var/run/docker.sock
             name: docker-socket
-          
         resources:
-            requests:
-              cpu: 0.2
-              memory: 100Mi
+          requests:
+            cpu: 0.2
+            memory: 100Mi
         imagePullPolicy: IfNotPresent
         env:
         - name: BUILD_NAMESPACE
@@ -1901,7 +1899,6 @@
             secretKeyRef:
               name: binder-secret
               key: "binder.hub-token"
-        
         ports:
           - containerPort: 8585
             name: binder

so everything looks right!

@consideRatio
Copy link
Member Author

Thank you for the review @minrk! Good review idea to provide a diff like that!

@betatim betatim merged commit 7cedaa7 into jupyterhub:master Oct 29, 2020
@betatim
Copy link
Member

betatim commented Oct 29, 2020

The mybinder.org kubeval check now fails https://github.com/jupyterhub/mybinder.org-deploy/pull/1679/checks?check_run_id=1327653768 any idea why/which change here lead to that?

Line 72 is referenced in the kubeval output but I think that doesn't correspond to L72 in the deployment.yaml here :-/

{{- end }}
{{- with .Values.extraEnv }}
{{- . | toYaml | indent 8 }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the error. It must be nindent

Copy link
Member Author

@consideRatio consideRatio Oct 29, 2020

Choose a reason for hiding this comment

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

Resolved by #1178. Sorry for the trouble @betatim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants