chore(helm): Refactor loki health probes - #18575
Conversation
My local search for |
|
I only did the ones that clearly caused errors with the values files from ci. But you are probably right – all of them should be updated. EDIT: Or should they? 🤔 Clearly, all the "bad" uses should be improved, so a I'll try and split it up into different commits so you can review them individually and choose which ones belong here. |
44c893b to
b1c3ed2
Compare
b1c3ed2 to
313a814
Compare
|
I've reworked the PR now. In my opinion, only 68b815e4d and maybe 91d8d1c4c belongs here, but I pushed 32e22d6e4 just in case you think that belongs here. If you agree with me, I'll remove that commit and just keep the ones you want. |
32e22d6 to
a3e2585
Compare
jkroepke
left a comment
There was a problem hiding this comment.
Great PR! I personally like this pattern and should be used everywhere.
I just found one odd thing, it's optional here and maybe a separate PR worth.
Just comment what did you think.
jkroepke
left a comment
There was a problem hiding this comment.
Missing a Changelog entry and this PR feel more "feat(helm)" than "chore(helm)"
|
From my point of view, both is fine.
We should not over engineer this. In best case, all your PRs are included in the same Loki Release and there is zero difference for the end-user. |
|
Thanks! Then I'll revert the commit! Just a minute! |
This change refactors all existing uses of `.Values.loki.readinessProbe`
and `.Values.loki.livenessProbe` so that they treat the value `{}` as
meaning that the probe should be disabled.
Some components used `.Values.loki.livenessProbe` despite it not being
set in values.yaml. So if that value gets introduced with `{}` as the
default value, then it would break things without this refactoring.
Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>
Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>
9831450 to
b1147e1
Compare
Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>

What this PR does / why we need it:
Some components used
.Values.loki.livenessProbeand.Values.loki.startupProbedespite them being unset in values.yaml. This change adds those parameters.Introducing these parameters with
{}as the default value breaks the existing logic, since they previously had the valuenull(because they were unset).But we want to have
{}as the default value, notnull, to indicate that the parameter is an object.So therefore, this change also refactors the existing logic so it handles
{}properly.Which issue(s) this PR fixes:
Partially addresses #13678
Special notes for your reviewer:
Partially replaces #15359
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR