-
Notifications
You must be signed in to change notification settings - Fork 28
CU- 86c6pjw94Create a new webhook for HPA creation #474
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
base: master
Are you sure you want to change the base?
CU- 86c6pjw94Create a new webhook for HPA creation #474
Conversation
| timeoutSeconds: {{ .Values.capabilities.admissionController.mutatingWebhook.timeoutSeconds | default 5 }} | ||
| {{- end }} | ||
| {{- if .Values.capabilities.admissionController.hpaWebhook.enabled }} | ||
| - name: hpa.komodor.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.
I'm not sure, maybe we want to use hpa.rightsizing.com? Again, not sure, would love to hear your opinion here
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 it is directly related to rightsizing capabilities, yes I would use hpa.rightsizing.komodor.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.
@iOrcohen I think you renamed the original rule instead of the new one 😶
| sideEffects: None | ||
| timeoutSeconds: {{ .Values.capabilities.admissionController.mutatingWebhook.timeoutSeconds | default 5 }} | ||
| {{- end }} | ||
| {{- if .Values.capabilities.admissionController.hpaWebhook.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.
Do these values exist? I mean, this wouldn't render if .Values.capabilities.admissionController.hpaWebhook is null.
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 is 100% an issue for users that will upgrade using --reuse-values as the hpaWebhook will be null for them.
You might be able to use dig https://helm.sh/docs/chart_template_guide/function_list/#dig or empty function like we did here
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.
Also regarding this, add default value object to the values.yaml file
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.
@iOrcohen add default values
| timeoutSeconds: {{ .Values.capabilities.admissionController.mutatingWebhook.timeoutSeconds | default 5 }} | ||
| {{- end }} | ||
| {{- if .Values.capabilities.admissionController.hpaWebhook.enabled }} | ||
| - name: hpa.komodor.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.
if it is directly related to rightsizing capabilities, yes I would use hpa.rightsizing.komodor.com
| sideEffects: None | ||
| timeoutSeconds: {{ .Values.capabilities.admissionController.mutatingWebhook.timeoutSeconds | default 5 }} | ||
| {{- end }} | ||
| {{- if .Values.capabilities.admissionController.hpaWebhook.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 is 100% an issue for users that will upgrade using --reuse-values as the hpaWebhook will be null for them.
You might be able to use dig https://helm.sh/docs/chart_template_guide/function_list/#dig or empty function like we did here
| sideEffects: None | ||
| timeoutSeconds: {{ .Values.capabilities.admissionController.mutatingWebhook.timeoutSeconds | default 5 }} | ||
| {{- end }} | ||
| {{- if .Values.capabilities.admissionController.hpaWebhook.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.
Also regarding this, add default value object to the values.yaml file
|
@OrBin @MickaelAlliel If we would want to open the HPA for a customer after release it, changing it in the remote config won't be enough, we would require the client to reinstall the Helm with a true value in it right? So I think maybe the webhook should exist anyway, and inside the AC we will check this remote configuration to understand if we want to do something with it. |
Correct yes - the best way for now is to have it deployed and the remote configuration will decide if to return early or actually do any logic. For reference, we didnt get to it yet but: |
That would be amazing, this with the fact we can changing the remote config and the AC server is restarting is pretty amazing |
Add webhook for HPA creation