Skip to content

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jul 17, 2021

This simplifies the one-sensor-per-pod method by pulling default values from st2sensorcontainer.

In other words, this makes st2sensorcontainer provide defaults for each pod in st2.packs.sensors.

@cognifloyd cognifloyd added feature Helm size/M PR that changes 30-99 lines. Good size to review. RFR labels Jul 17, 2021
@cognifloyd cognifloyd requested a review from arm4b July 18, 2021 01:10
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.
@cognifloyd
Copy link
Member Author

@armab This is ready for review.

Aside - why didn't pull-request-size bot label this PR? I went ahead and added the size/M label myself.

Copy link
Member Author

@cognifloyd cognifloyd left a 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.

labels:
app: st2sensorcontainer{{ template "hyphenPrefix" .name }}
app: {{ $name }}
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 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) }}
Copy link
Member Author

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.

Comment on lines +985 to +988
{{- $sensor := omit $.Values.st2sensorcontainer "name" "ref" "postStartScript" }}
{{- range $key, $val := . }}
{{- $_ := set $sensor $key $val }}
{{- end }}
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 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.

Comment on lines +489 to +491
requests:
memory: "100Mi"
cpu: "50m"
Copy link
Member Author

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]

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks!

@arm4b arm4b merged commit fd2e6db into StackStorm:master Jul 27, 2021
@cognifloyd cognifloyd removed the RFR label Jul 27, 2021
@cognifloyd cognifloyd deleted the sensor-defaults branch November 11, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Helm size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants