Helm: Pass etcd_cluster_name to command args. Allow service annotations #500
Conversation
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
=======================================
Coverage 71.01% 71.01%
=======================================
Files 37 37
Lines 2201 2201
=======================================
Hits 1563 1563
Misses 575 575
Partials 63 63Continue to review full report at Codecov.
|
sebito91
left a comment
sebito91
left a comment
There was a problem hiding this comment.
Thank you very much for your PR!
A few points of note on the overall consistency of the changes:
- overall I'd like to keep the CLI portion as clear as possible, so setting annotations there can be confusing. I like the idea of making the attributes as variables but we should keep them all in the same place, even if you can override from the CLI (most users won't)
- we'll need to have a think later on how to make the
db-hostsportion something that's a comma-delimited list of nodes instead of just a single reference (you don't have to do this now, just thinking aloud)
Overall nicely done, thanks again for the addition!
contrib/helm/README.md
Outdated
| | `resources.requests.memory` | Memory resource requests | `256Mi` | | ||
| | `command` | Options to pass to `event-gateway` command | `[-db-hosts=eg-etcd-cluster-client:2379, -log-level=debug]`| | ||
| | `etcd_cluster_name` | Name of the etcd cluster. Must be passed to the `-db-host` option as `<etcd-cluster-name>-client` | `eg-etcd-cluster`| | ||
| | `etcd_cluster_name` | Name of the etcd cluster. Passed to the `-db-host` option as `<etcd-cluster-name>-client` | `eg-etcd-cluster`| |
There was a problem hiding this comment.
Please update to -db-hosts
| image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| args: | ||
| - "-db-hosts={{ .Values.etcd_cluster_name }}-client:2379" |
There was a problem hiding this comment.
Since we're at it, let's please make the port a dynamic variable too. {{ Values.ectd_cluster_port }}
We should really look to amend this for future as technically this is a comma-delimited list of <host>:<port> combinations for the kv cluster. We happen to be using etcd at the moment, but the idea here is that this can be swapped out as needed.
There was a problem hiding this comment.
Done. And +1 on the longer-term plan.
contrib/helm/README.md
Outdated
| The service annotations can be used to set any annotations required by your platform, for example: | ||
|
|
||
| ``` | ||
| $ helm install . --name eg --set service.annotations="{service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0,foo: bar}" --debug --dry-run | grep "kind: Service" -A5 |
There was a problem hiding this comment.
I'm a little wary of this as it's entirely dependent on individual use-case. We completely understand the need to customize deployments based on environment but I think instructions should default to setting the values.yml file instead of the command line as much as possible.
Can you please update these instructions to add clarity on defining the variable in the values file and how to list them here? Also, please keep it consistent and name the folder you're installing, like helm install event-gateway ....
There was a problem hiding this comment.
Yeah, I was unsure about including it at all in the README, given it's customization beyond the basic port/limit etc. I've updated it as you mentioned, using an example of updating the values, but let me know if you'd prefer to just remove the paragraph.
absoludity
left a comment
absoludity
left a comment
There was a problem hiding this comment.
Thanks for the feedback.
contrib/helm/README.md
Outdated
| | `resources.requests.memory` | Memory resource requests | `256Mi` | | ||
| | `command` | Options to pass to `event-gateway` command | `[-db-hosts=eg-etcd-cluster-client:2379, -log-level=debug]`| | ||
| | `etcd_cluster_name` | Name of the etcd cluster. Must be passed to the `-db-host` option as `<etcd-cluster-name>-client` | `eg-etcd-cluster`| | ||
| | `etcd_cluster_name` | Name of the etcd cluster. Passed to the `-db-host` option as `<etcd-cluster-name>-client` | `eg-etcd-cluster`| |
contrib/helm/README.md
Outdated
| The service annotations can be used to set any annotations required by your platform, for example: | ||
|
|
||
| ``` | ||
| $ helm install . --name eg --set service.annotations="{service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0,foo: bar}" --debug --dry-run | grep "kind: Service" -A5 |
There was a problem hiding this comment.
Yeah, I was unsure about including it at all in the README, given it's customization beyond the basic port/limit etc. I've updated it as you mentioned, using an example of updating the values, but let me know if you'd prefer to just remove the paragraph.
| image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| args: | ||
| - "-db-hosts={{ .Values.etcd_cluster_name }}-client:2379" |
There was a problem hiding this comment.
Done. And +1 on the longer-term plan.
sebito91
left a comment
sebito91
left a comment
There was a problem hiding this comment.
This is great stuff, thanks for fixing the issues!
2 participants
What did you implement:
Small improvements to provided helm template after testing the existing one. I found it a little confusing (and unnecessary) to have to update the
commandoption if you changedetcd_cluster_name. Also wasn't able to deploy in my k8s env without a specific service annotation, so added this option (but can remove if it's too noisy).Relates to #448
How did you implement it:
How can we verify it:
Test the helm chart as per the README.md
Todos:
Is this ready for review?: Yes, perhaps @sebito91 based on the previous PR. Also cc @baniol in case he has other thoughts (and thanks for adding it in the first place!)
Is it a breaking change?: No