-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
[bitnami/argo-cd] fix: provide a functional validation function for validation #29724
Conversation
Hey @mFranz82 , Ypou can find more info on that specific change in this discussion: #27585 (comment) The main reason for the change is, that before it did nothing. The messages were collected but never printed. The default behavior should be the failing of the chart, as stated by @fmulero :
Imho the issue is, that the validation should only yield the message when redis is not enabled AND externalRedis is also not enabled:
Side-Note: The validation for redis seems a bit weird:
|
Hi @maxnitze
Do we want to merge it like this or do you think we should fundamentally revise the rules? |
I have a couple of smaller issues with the current setup. And your fix does not address them all.
This is a valid setup, from your validation: redis:
enabled: false
externalRedis:
enabled: false
host: some-host
port: 1337
Is it a valid case, that both IMHO the validation could be something like this: {{/*
Validate Redis config
*/}}
{{- define "argocd.validateValues.redis" -}}
{{- if and .Values.redis.enabled .Values.externlaRedis.enabled }}
Argo CD: You must not enable both, redis dependency and external redis
{{- end -}}
{{- if and .Values.redis.enabled .Values.redis.auth.existingSecret }}
{{- if not .Values.redis.auth.existingSecretPasswordKey -}}
Argo CD: You need to provide existingSecretPasswordKey when an existingSecret is specified in redis dependency
{{- end -}}
{{- else if and (not .Values.redis.enabled) .Values.externalRedis.existingSecret }}
{{- if not .Values.externalRedis.existingSecretPasswordKey -}}
Argo CD: You need to provide existingSecretPasswordKey when an existingSecret is specified in redis
{{- end }}
{{- end -}}
{{- end -}}
{{- define "argocd.validateValues.externalRedis" -}}
{{- if .Values.externalRedis.enabled -}}
{{- if not .Values.externalRedis.port -}}
Argo CD: If the redis dependency is disabled you need to add an external redis port
{{- end -}}
{{- if not .Values.externalRedis.host -}}
Argo CD: If the redis dependency is disabled you need to add an external redis host
{{- end -}}
{{- end -}}
{{- end -}} TL;DR: added a block to validate only one of the two is enabled in the first helper and changed the second helper to check whether What do you think about that? I still find the separation between these two a bit confusing (the first checks whether the secrets are correctly configured for both cases although the second "looks" like it would do all the checks for externalRedis), but I hope you get the gist :) |
Hi there, Thank you for the contribution and all the detailed discussion! This PR looks good to me as it is, although, as @maxnitze mentions this setup would skip the validation and fail at startup:
Now, IMHO it is the value All of our charts use My idea of a complete fix for this would be to remove the Let me know what you think before I merge this PR, thanks! |
1b5887e
to
d57e7b4
Compare
Hi @migruiz4 As @maxnitze suggested I finally changed the validation rule for external redis to this:
I think we should leave it as it is for now and revise it at a later date... |
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.
Please take a look at my comment
@@ -288,8 +288,13 @@ Argo CD: You need to provide existingSecretPasswordKey when an existingSecret is | |||
Validate external Redis config | |||
*/}} | |||
{{- define "argocd.validateValues.externalRedis" -}} | |||
{{- if not .Values.redis.enabled -}} | |||
{{- if not .Values.externalRedis.enabled -}} |
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 one line would be very problematic because it won't allow deploying with default values (redis subchart):
$ helm install --dry-run argo-cd bitnami/argo-cd
Error: INSTALLATION FAILED: execution error at (argo-cd/templates/NOTES.txt:60:4):
VALUES VALIDATION:
Argo CD: If the redis dependency is disabled you need to add an external redis host
My suggestion would be to revert to:
{{- if not .Values.externalRedis.enabled -}} | |
{{- if not .Values.redis.enabled -}} |
Or alternatively, if we want to consider the value externalRedis.enabled
, then:
{{- if not .Values.externalRedis.enabled -}} | |
{{- if and .Values.externalRedis.enabled (not .Values.redis.enabled) -}} |
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.
Hey @migruiz4
Fortunately you discovered the error 👍
I recently revert the change I made!
Sorry and thanks!
… redis settings Signed-off-by: Martin Franz <info@martin-fra.nz>
b6c19dc
to
02a4a32
Compare
Signed-off-by: Bitnami Containers <bitnami-bot@vmware.com>
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.
Looks good to me! Thank you very much!
Description of the change
Since a change made 3 month before the templating fails if using
redis.enabled=false
The error is triggered at this point in Notes.txt
And occurs because the validation function could never be successful:
As you can see in the snippet above the validating function always fails if
redis.enabled = false
Benefits
You should be able again to install the chart without using internal redis.
Possible drawbacks
No
Applicable issues
No
Additional information
Generally I think that the validation should just produce warnings and the templating should not fail. I am not sure why @maxnitze introduced the pattern.
Checklist
Chart.yaml
according to semver. This is not necessary when the changes only affect README.md files.README.md
using readme-generator-for-helm