-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove .Values.service check before setting serviceName #512
Conversation
Increases Helm 3 compatibility, in line with other StatefulSet definitions that are known to work (at a basic level) with Helm 3, such as helm-charts/elasticsearch/templates/statefulset.yaml#L16
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
💚 CLA has been signed |
Just signed the Contributor Agreement. |
cla/check |
jenkins test this please |
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.
Looking - thanks! |
Updated test & checked that no other serviceName checks are performed. I'm not entirely sure why this check was present, so I'm interested to see if Helm 2 functional testing works 100% as expected. |
jenkins test this please |
Looks like no dice on Logstash + ES 6, per Jenkins. Though this is for Logstash + ES 7, so I don't know if that's acceptable or not. |
@tweedge Logstash 6 test is failing for some strange reason ( Note that with this change, Can you update We'll see if that fix the strange Logstash 6 behavior. |
jenkins test this please |
Thanks for working on this. Any updates on the last test? |
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist. |
Hi @tweedge, @aidan-melen, I'm closing this one. Thanks for your help 👍 |
It appears that {{-if .Values.service }} is evaluating falsely here, preventing a required value in Helm 3 from being set (serviceName), and making this change resolves #497 without any other modifications to the template. Other StatefulSet definitions that are known to work (at a basic level) with Helm 3, such as elasticsearch/templates/statefulset.yaml#L16, do not implement this check.
It seems that making this change allows for additional Helm 3 compatibility without breaking anything in Helm 2, though correct me if I'm wrong there - I don't have anything running Helm 2.
${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml
^ Checked off all of the above as this change impacts none of them.