-
-
Notifications
You must be signed in to change notification settings - Fork 117
Make st2.packs.sensors pull default values from st2sensorcontainer #221
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
Conversation
a579799 to
497b218
Compare
497b218 to
f033c89
Compare
9d73c9b to
9027a0f
Compare
merge does a deep merge, but that could cause weird problems if two different livenessProbe definitions were merged and other things that would not make sense. Instead, we use set to mutate the $sensor dictionary applying the override values on top of the defaults, effectively merging only one level of the dictionaries.
a20109c to
de6264c
Compare
|
@armab This is ready for review. Aside - why didn't |
cognifloyd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armab here are a few comments explaining "why" on several parts of the PR.
| name: {{ $.Release.Name }}-{{ $name }} | ||
| labels: | ||
| app: st2sensorcontainer{{ template "hyphenPrefix" .name }} | ||
| app: {{ $name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This chunk was repeated on 5 lines, each of which has to be changed to use $sensor.name. So, I consolidated the name definition into one line (above).
| {{- range $key, $val := . }} | ||
| {{- $_ := set $sensor $key $val }} | ||
| {{- end }} | ||
| {{- $name := print "st2sensorcontainer" (include "hyphenPrefix" $sensor.name) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $name variable is only available (in-scope) inside the sensor range block.
| {{- $sensor := omit $.Values.st2sensorcontainer "name" "ref" "postStartScript" }} | ||
| {{- range $key, $val := . }} | ||
| {{- $_ := set $sensor $key $val }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies the default. I looked at using one of the merge functions to generate the $sensor dictionary, but all of the merge functions are deep merge functions which is undesirable. Using set like this allows us to replace keys instead of deep merging them.
| requests: | ||
| memory: "100Mi" | ||
| cpu: "50m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied these defaults from what is currently in st2.packs.sensors[0]
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This simplifies the one-sensor-per-pod method by pulling default values from
st2sensorcontainer.In other words, this makes
st2sensorcontainerprovide defaults for each pod inst2.packs.sensors.