[metricbeat] split values for daemonset and deployment #572

jmlrt · 2020-04-10T16:49:03Z

Split values for DaemonSet and Deployment:

Fixes #446
Replace #448

Chart version not bumped (the versions are all bumped and released at the same time)
README.md updated with any new values or changes
Updated template tests in ${CHART}/tests/*.py
Updated integration tests in ${CHART}/examples/*/test/goss.yaml

* reorder some values * improve values description

mgreau

LGTM

mgreau · 2020-04-17T03:22:18Z

metricbeat/README.md

+[serviceAccount]: https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/
+[tolerations]: https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/
+[updateStrategy]: https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set/#daemonset-update-strategy
+[values.yaml]: https://github.com/elastic/helm-charts/tree/master/metricbeat/values.yaml


New line missing

fixed in b489901

mgreau · 2020-04-17T03:27:13Z

metricbeat/templates/configmap.yaml

+  {{ $path }}: |
+{{ $config | indent 4 -}}
+{{- end -}}
+{{- end -}}


New line missing

fixed in b489901

Crazybus

LGTM!

This is fantastic work! As the person that originally created this mess...thank you so much for fixing this up and making everything else better in the process :)

I thought I found an issue, but after testing it looks like it works as expected. I just hadn't seen that syntax before. I left the comments in anyway because it confused me until I looked up how it worked.

Crazybus · 2020-04-17T12:49:09Z

metricbeat/templates/daemonset.yaml

@@ -136,6 +130,13 @@ spec:
          mountPath: /usr/share/metricbeat/{{ $path }}
          readOnly: true
          subPath: {{ $path }}
+        {{ else }}


🚫 I think this else should be an {{ end }} and the other end below needs to be removed. The range loop above is going over .Values.metricbeatConfig and doesn't seem to end properly.

Suggested change

{{ else }}

{{ end }}

So this looked like an issue to me but I couldn't make it cause any unexpected issues when testing. After looking at the docs it looks like you can do a {{ range }} {{ else }}. I think the syntax just through me off:

{{range pipeline}} T1 {{else}} T0 {{end}}
The value of the pipeline must be an array, slice, map, or channel.
If the value of the pipeline has length zero, dot is unaffected and
T0 is executed; otherwise, dot is set to the successive elements
of the array, slice, or map and T1 is executed.

Yeah {{ range }} {{ else }} isn't documented in Helm doc, but is fine in Go template. That said, if you think this syntax can be confusing or lead to mistake, I'm fine with replacing it by 2 entirely different {{ range }} loops instead.

Crazybus · 2020-04-17T12:50:51Z

metricbeat/templates/deployment.yaml

@@ -32,17 +32,16 @@ spec:
        heritage: '{{ .Release.Service }}'
        release: '{{ .Release.Name }}'
    spec:
-      {{- with .Values.tolerations }}


❤️ I really like all of these little cleanups here to add some consistency. I like the much cleaner and shorter versions using nindent below :)

Yeah Helm templates are a little more readable with nindents 😃.

Crazybus · 2020-04-17T12:54:56Z

metricbeat/templates/deployment.yaml

-{{- if .Values.podSecurityContext }}
-        securityContext:
-{{ toYaml .Values.podSecurityContext | indent 10 }}
+{{- if .Values.envFrom | default .Values.deployment.envFrom }}


💡 I think this if statement can just be dropped. Since the default is an empty list already.

fixed in 890428d

Crazybus · 2020-04-17T12:58:01Z

metricbeat/tests/metricbeat_test.py

+
+
+def test_adding_deprecated_envs():
+    config = """
 extraEnvs:


❤️ Great to see this test being added to test the backwards compatibility logic!

Crazybus · 2020-04-17T13:09:06Z

metricbeat/templates/daemonset.yaml

+          mountPath: /usr/share/metricbeat/{{ $path }}
+          readOnly: true
+          subPath: {{ $path }}
+        {{- end }}


Suggested change

{{- end }}

This comment was part of #572 (comment) and can be ignored.

Crazybus

LGTM!

[metricbeat] split values for daemonset and deployment

jmlrt · 2020-04-20T14:47:53Z

backported to 7.7 branch in f74d721

…624) This commit fix a bug introduced in #572 where Metricbeat deployment doesn't start after an upgrade from a previous version when using a custom metricbeatConfig.

jmlrt added the enhancement New feature or request label Apr 10, 2020

jmlrt force-pushed the split-metricbeat-values branch from b9a10e7 to 03a0cb7 Compare April 10, 2020 16:58

jmlrt mentioned this pull request Apr 10, 2020

split some of the deployment and daemonset config to make things more… #448

Closed

4 tasks

[metricbeat] split configmap for daemonset and deployment

cea4e9a

jmlrt force-pushed the split-metricbeat-values branch from 03a0cb7 to cea4e9a Compare April 10, 2020 17:39

[metricbeat] use markdown implicit link name for config table

61c96be

jmlrt force-pushed the split-metricbeat-values branch from 04037cf to 61c96be Compare April 14, 2020 15:08

jmlrt added 2 commits April 15, 2020 00:31

[metricbeat] split affinity for daemonset and deployment

2a5e45d

[metricbeat] split resources for daemonset and deployment

c8af71b

jmlrt force-pushed the split-metricbeat-values branch from d7383ff to c8af71b Compare April 14, 2020 22:32

jmlrt added 3 commits April 15, 2020 00:57

[metricbeat] split nodeSelector for daemonset and deployment

f667825

[metricbeat] split tolerations for daemonset and deployment

bcdab45

[metricbeat] nit

7914ffb

* reorder some values * improve values description

jmlrt force-pushed the split-metricbeat-values branch from c6386ab to 7914ffb Compare April 15, 2020 09:16

[metricbeat] split security context for daemonset and deployment

07877a6

jmlrt force-pushed the split-metricbeat-values branch from d6e3741 to 07877a6 Compare April 15, 2020 15:22

[metricbeat] nit - fix default values in README

d1a96a1

jmlrt force-pushed the split-metricbeat-values branch from ce45c1a to 12d6ffe Compare April 15, 2020 19:06

jmlrt added 3 commits April 15, 2020 22:21

metricbeat] split extraEnvs for daemonset and deployment

bc05368

[metricbeat] split envFrom for daemonset and deployment

1dd2e84

[metricbeat] split volumes and volumeMounts for daemonset and deployment

593c743

jmlrt force-pushed the split-metricbeat-values branch from 89e9d16 to 593c743 Compare April 15, 2020 20:23

[metricbeat] split secretMounts for daemonset and deployment

055313c

jmlrt requested review from Crazybus and a team April 15, 2020 20:47

jmlrt marked this pull request as ready for review April 15, 2020 20:48

mgreau previously approved these changes Apr 17, 2020

View reviewed changes

Crazybus previously approved these changes Apr 17, 2020

View reviewed changes

jmlrt added 2 commits April 17, 2020 16:52

[metricbeat] remove unneeded if statement for envFrom

890428d

[metricbeat] nit - add missing trailing lines

b489901

jmlrt dismissed mgreau’s stale review via b489901 April 17, 2020 15:00

jmlrt requested review from Crazybus and mgreau April 17, 2020 15:00

Crazybus approved these changes Apr 17, 2020

View reviewed changes

jmlrt merged commit 2c82a65 into elastic:master Apr 17, 2020

jmlrt deleted the split-metricbeat-values branch April 17, 2020 15:31

jmlrt added the v7.7.0 label Apr 17, 2020

mgreau pushed a commit that referenced this pull request Apr 17, 2020

Merge pull request #572 from jmlrt/split-metricbeat-values

f74d721

[metricbeat] split values for daemonset and deployment

simonme mentioned this pull request May 12, 2020

Metricbeat Beat failed to connect es (xpack enabled) #604

Closed

jmlrt mentioned this pull request Oct 28, 2020

fix 854 68 #876

Closed

This was referenced Nov 17, 2020

68 helm 3 #900

Closed

7x helm 3 #901

Closed

7.10 init #913

Closed

710 readme enhancements #926

Closed

jmlrt mentioned this pull request Feb 8, 2021

backport 1046 7x #1051

Closed

This was referenced Mar 15, 2021

7.12 #1101

Closed

7.x #1113

Closed

jmlrt mentioned this pull request May 25, 2021

7.x bump #1199

Closed

This was referenced Dec 14, 2021

[meta] initiate 7.16 branch #1475

Merged

[kibana] optionally disable SA token automount (#1301) #1481

Merged

6.8.21 bump #1488

Merged

jmlrt mentioned this pull request Mar 8, 2022

7.17.1 bump #1609

Merged

jmlrt mentioned this pull request Apr 21, 2022

7.17.3 bump #1649

Merged

This was referenced Sep 14, 2022

[meta] fix yaml indent #1698

Merged

[meta] follow-up of #1698 #1700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metricbeat] split values for daemonset and deployment #572

[metricbeat] split values for daemonset and deployment #572

jmlrt commented Apr 10, 2020 •

edited

Loading

mgreau left a comment

mgreau Apr 17, 2020

jmlrt Apr 17, 2020

mgreau Apr 17, 2020

jmlrt Apr 17, 2020

Crazybus left a comment

Crazybus Apr 17, 2020

Crazybus Apr 17, 2020

jmlrt Apr 17, 2020

Crazybus Apr 17, 2020

jmlrt Apr 17, 2020

Crazybus Apr 17, 2020

jmlrt Apr 17, 2020

jmlrt Apr 17, 2020

Crazybus Apr 17, 2020

Crazybus Apr 17, 2020

Crazybus Apr 17, 2020

Crazybus left a comment

jmlrt commented Apr 20, 2020

[metricbeat] split values for daemonset and deployment #572

[metricbeat] split values for daemonset and deployment #572

Conversation

jmlrt commented Apr 10, 2020 • edited Loading

mgreau 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

Crazybus 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

Crazybus left a comment

Choose a reason for hiding this comment

jmlrt commented Apr 20, 2020

jmlrt commented Apr 10, 2020 •

edited

Loading