-
Notifications
You must be signed in to change notification settings - Fork 5
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
allow empty status informers #102
Conversation
8a7ba41
to
5c9b2a0
Compare
statusInformers: [] | ||
EOF | ||
|
||
output=$(helm template oci://ttl.sh/automated-${{ github.run_id }}/replicated --version 0.0.0 --values test-values.yaml) |
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.
nit pick: any reason not to use --set replicated.statusInformers=[]
?
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.
that ended up rendering this:
statusInformers:
- "[]"
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.
huh, that's not what Helm's docs say. It's fine then, let's leave it as is.
https://helm.sh/docs/intro/using_helm/#the-format-and-limitations-of---set
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.
saw that too. maybe it's because of how we template it
$ helm template replicated chart/replicated-0.0.0.tgz --set statusInformers=[] | grep -i statusInformers -A1
statusInformers:
'[]'
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.
yeah, probably. hmm, this might break in a production install if someone decides to override status informers using helm set
, right?
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.
if they try to override it using --set replicated.statusInformers=[]
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 key issue is that {{- if .Values.statusInformers }}
evaluates to true
when using --set replicated.statusInformers=[]
, but evaluates to false
when passing the same via a user-provided values file. looking like Helm is interpreting []
as a string in the former case and as an array in the latter.
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.
Why not use if not (kindIs "invalid" ...
like we do with the integration.enabled
field?
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 result is the same in that case as it's interpreted as a string when --set replicated.statusInformers=[]
is passed to the cli:
$ helm template replicated chart/replicated-0.0.0.tgz --set statusInformers=[] | grep -i statusInformers -A1
statusInformers:
'[]'
0f22def
to
5d5c759
Compare
5d5c759
to
0fba0f9
Compare
What this PR does / why we need it:
This PR allows an empty array of status informers to be explicitly passed to the SDK. Previously if this was the case, the SDK would automatically generate status informers based on the Helm release. The default value for
statusInformers
will still result in them being automatically generated, which maintains existing behavior.Which issue(s) this PR fixes:
Part of https://app.shortcut.com/replicated/story/88171/detect-and-configure-the-replicated-sdk-chart-subchart-if-it-s-part-of-the-vendor-s-application
Special notes for your reviewer:
Steps to reproduce
Does this PR introduce a user-facing change?
Does this PR require documentation?
NONE