arttor / helmify

Creates Helm chart from Kubernetes yaml
MIT License
1.48k stars 136 forks source link

Open to adding args/extraArgs? #108

Closed foot closed 1 year ago

foot commented 1 year ago

Awesome project!

We're using helmify in a kubebuilder project as mentioned in the README.md.

Now we're looking into ways to allow the user to configure the controller a bit more when they install the helm chart. I'm not sure what the most idiomatic patterns for doing this but args is one that comes to mind. Would the helmify project be open to a change like that?

For example:

I imagine it work similarly to the securityContext extraction already in place.

Happy to put together a PR if this makes sense?

arttor commented 1 year ago

Hi, we can definitely think about adding a new flag since we already have a couple of them. Please describe which spec needs to be templated. Maybe we should template it by default without adding the flag.

On the other hand, it is not the best way to control too many specs with flags. Please check out #59 - having a config is a better idea but the problem is that this feature is quite big compared to quckfix with a flag.

foot commented 1 year ago

Ah sorry! I actually meant args/extraArgs for a pod, not for the helmify cli.

But as you say you may not want this behaviour for some reason and so would want to disable it via a flag to helmify too..

Proposed changes would look like this https://github.com/arttor/helmify/compare/main...bigkevmcd:helmify:args

The diff from 0.4.1 and that branch using cat test_data/sample-app.yaml | helmify looks like this

diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml
index f23af3a..89a8a87 100644
--- a/chart/templates/deployment.yaml
+++ b/chart/templates/deployment.yaml
@@ -18,10 +18,7 @@ spec:
       {{- include "chart.selectorLabels" . | nindent 8 }}
     spec:
       containers:
-      - args:
-        - --health-probe-bind-address=:8081
-        - --metrics-bind-address=127.0.0.1:8080
-        - --leader-elect
+      - args: {{- toYaml .Values.myapp.app.args | nindent 10 }}
         command:
         - /manager
         env:
@@ -65,9 +62,7 @@ spec:
           name: props
         - mountPath: /usr/share/nginx/html
           name: sample-pv-storage
-      - args:
-        - --secure-listen-address=0.0.0.0:8443
-        - --v=10
+      - args: {{- toYaml .Values.myapp.proxySidecar.args | nindent 10 }}
         env:
         - name: KUBERNETES_CLUSTER_DOMAIN
           value: {{ quote .Values.kubernetesClusterDomain }}
diff --git a/chart/values.yaml b/chart/values.yaml
index c7e5a6e..b26d4a1 100644
--- a/chart/values.yaml
+++ b/chart/values.yaml
@@ -1,11 +1,13 @@
 batchJob:
   backoffLimit: 4
   pi:
+    args: []
     image:
       repository: perl
       tag: 5.34.0
 cronJob:
   hello:
+    args: []
     image:
       repository: busybox
       tag: "1.28"
@@ -13,6 +15,7 @@ cronJob:
   schedule: '* * * * *'
 fluentdElasticsearch:
   fluentdElasticsearch:
+    args: []
     image:
       repository: quay.io/fluentd_elasticsearch/fluentd
       tag: v2.5.2
@@ -53,6 +56,10 @@ mySecretVars:
   var2: ""
 myapp:
   app:
+    args:
+    - --health-probe-bind-address=:8081
+    - --metrics-bind-address=127.0.0.1:8080
+    - --leader-elect
     containerSecurityContext:
       allowPrivilegeEscalation: false
     image:
@@ -69,6 +76,9 @@ myapp:
     region: east
     type: user-node
   proxySidecar:
+    args:
+    - --secure-listen-address=0.0.0.0:8443
+    - --v=10
     image:
       repository: gcr.io/kubebuilder/kube-rbac-proxy
       tag: v0.8.0
@@ -92,6 +102,7 @@ pvc:
     storageRequest: 3Gi
 web:
   nginx:
+    args: []
     image:
       repository: registry.k8s.io/nginx-slim
       tag: "0.8"
arttor commented 1 year ago

looks nice, i think it is a good idea to template container args, especially for Jobs and CronJobs

arttor commented 1 year ago

@bigkevmcd @foot thank you for your contribution! The changes are available in v0.4.2 release.