-
Notifications
You must be signed in to change notification settings - Fork 363
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
[velero] Allow to define resource requests/limits for the containers in the upgrade/cleanup job #305
Conversation
…rs in the upgrade/cleanup job. Signed-off-by: Daniel Simionato <[email protected]>
Signed-off-by: Daniel Simionato <[email protected]>
@@ -53,6 +53,10 @@ spec: | |||
kubectl delete volumesnapshotlocation --all; | |||
kubectl delete podvolumerestore --all; | |||
kubectl delete crd -l app.kubernetes.io/name=velero; | |||
{{- with .Values.kubectl.resources }} |
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.
Should these additions be put behind a conditional so that empty "resources" fields aren't added? I'm not 100% sure that it matters, but that way there's no change in yaml for cases where the optional resources aren't specified:
{{- if .Values.kubectl.resources }}
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.
Hi,
thanks for the review.
I had the same concern but testing with helm template if resources is empty (either resources: {}
like in values.yaml or simply resources:
) nothing gets included in the final manifest. Basically, if there's something defined it gets converted in yaml and indented with 12 spaces; if there's nothing, nothing gets added as expected.
The same style of inclusion is used freely in the chart ( e.g. for the resources of the main deployment here
{{- with .Values.resources }} |
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 prefer the with
syntax personally.
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.
LGTM
Thanks for the contribution @weseven
This PR allows to define resource requests and limits for the containers in the upgrade and cleanup jobs.
This is necessary in kubernetes clusters where an admission controller prevents the creation of pods without proper resource requests and limits.
Checklist
[velero]
)