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

Adds default selectorLabels to topologySpreadConstraints if not otherwise specified. #429

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

bderrly
Copy link

@bderrly bderrly commented Nov 26, 2024

If someone defines one or more entries in the topologySpreadConstraints list and does not define the labelSelectors.matchLabels keys then default values will be added. Most people are likely going to want the default selectorLabels so the spread constraints match what they are deploying with this chart.

The alternative to this is to run the configuration in this key through the tpl function to allow users the ability to call named templates. Similar to how extraObjects is handled.

If someone defines one or more entries in the
`topologySpreadConstraints` list and does not define the
`labelSelectors.matchLabels` keys then default values will be added.
Most people are likely going to want the default selectorLabels so the
spread constraints match what they are deploying with this chart.
@bderrly bderrly force-pushed the default-topology-constraints branch from 076c5ec to ed379a9 Compare November 26, 2024 23:54
@@ -174,6 +174,13 @@ tolerations:
{{- end }}
{{- with .Values.topologySpreadConstraints }}
topologySpreadConstraints:
{{- range $_, $entry := . }}
{{- if not (dig "labelSelectors" "matchLabels" false $entry) }}
{{- $ls := "labelSelectors:\n matchLabels: {}" | fromYaml }}
Copy link
Member

Choose a reason for hiding this comment

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

Not very familiar with this, but the \n seems a bit hacky. I wonder if we can do this more cleanly with a dict.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, it's kind of gross. I just updated this to use the dict function to create the nested dictionary keys.

Copy link
Member

@pront pront left a comment

Choose a reason for hiding this comment

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

Thanks @bderrly.
cc @jszwedko does this make sense to you?

@jszwedko
Copy link
Member

jszwedko commented Dec 2, 2024

I'm afraid I don't know enough about topologySpreadConstraints to comment 😓

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.

3 participants