Skip to content
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

feat: added priorityClassName to all charts #638

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

WatcherWhale
Copy link
Contributor

At the moment, only the hydra and hydra-maester charts allow configuring the priorityClassName. This pull request introduces this configuration option to the other charts as well.

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there!
Thanks for the PR, just adding some housekeeping, and then we can merge :)

@@ -425,6 +429,10 @@ statefulSet:
# lines, adjust them as necessary, and remove the curly braces after 'labels:'.
# e.g. type: app

# -- Pod priority
# https://kubernetes.io/docs/concepts/configuration/pod-priority-preemption/
# priorityClassName: ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# priorityClassName: ""
priorityClassName: ""

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty string is evaluated to false in both if and with clauses, so we can safely use it as a default value :)

@@ -202,6 +202,9 @@ spec:
{{- with $extraVolumes }}
{{- toYaml . | nindent 8 }}
{{- end }}
{{- if .Values.priorityClassName }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For internal consistency please use the with method instead of if :)

@@ -29,6 +29,10 @@ sidecar:
tag: v0.1.2
envs: {}

# -- Pod priority
# https://kubernetes.io/docs/concepts/configuration/pod-priority-preemption/
priorityClassName: ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please update the values.yaml files in the hack/values directory to set those fields. This allows the CI to verify if they generate valid manifest files

Signed-off-by: Mathias Maes <mathias.maes@aloxy.io>
Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Demonsthere Demonsthere merged commit d4872e4 into ory:master Sep 7, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants