-
Notifications
You must be signed in to change notification settings - Fork 30
[PM-19673] Add nodeSelector and tolerations #239
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
[PM-19673] Add nodeSelector and tolerations #239
Conversation
Thank you for your contribution! We've added this to our internal Community PR board for review. |
tolerations should be array of objects
I forgot to update
|
Great job, no security vulnerabilities found in this Pull Request |
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.
Pull Request Overview
Adds support for specifying nodeSelector
and tolerations
across both the sm-operator and self-host Helm charts.
- Extended the values schema in both charts to include
nodeSelector
andtolerations
with sensible defaults. - Updated
values.yaml
for self-host to define these defaults at global and per-component levels. - Injected conditional blocks in all relevant templates to render
nodeSelector
andtolerations
into pod specs.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
charts/sm-operator/values.schema.json | Added nodeSelector and tolerations under containers |
charts/sm-operator/templates/deployment.yaml | Injected nodeSelector /tolerations into deployment spec |
charts/self-host/values.yaml | Defined global and component defaults for selectors and tolerations |
charts/self-host/values.schema.json | Expanded schema with new properties for all components |
charts/self-host/templates/web.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/sso.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/scim.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/pre-install-secret-sql.yaml | Enabled selectors and tolerations in hook spec |
charts/self-host/templates/pre-install-job.yaml | Enabled selectors and tolerations in job spec |
charts/self-host/templates/pre-install-db-migrator-job.yaml | Enabled selectors and tolerations in migrator job spec |
charts/self-host/templates/post-install-db-migrator-job.yaml | Enabled selectors and tolerations in migrator job spec |
charts/self-host/templates/post-delete-hook.yaml | Enabled selectors and tolerations in hook spec |
charts/self-host/templates/notifications.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/mssql.yaml | Enabled selectors and tolerations for mssql pods |
charts/self-host/templates/identity.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/icons.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/events.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/attachments.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/api.yaml | Added conditional rendering of selectors and tolerations |
charts/self-host/templates/admin.yaml | Added conditional rendering of selectors and tolerations |
Comments suppressed due to low confidence (3)
charts/self-host/templates/web.yaml:28
- Because
general.nodeSelector
defaults to{}
(a non-nil map), thisif
condition always evaluates to true, resulting in an emptynodeSelector
block being rendered even when no selector is provided. Consider checking for non-empty values (e.g., usinglen
) or allowing null defaults.
{{- if or .Values.component.web.nodeSelector .Values.general.nodeSelector }}
charts/self-host/templates/sso.yaml:28
- [nitpick] This block is duplicated across many templates. Consider extracting a helper in
_helpers.tpl
to rendernodeSelector
andtolerations
, reducing repetition and easing future updates.
{{- if or .Values.component.sso.nodeSelector .Values.general.nodeSelector }}
@Invincibear , working through some CI blockers, we will get this merged once CI passes 😄 . |
|
🎟️ Tracking
This is a re-implementation of #168, a PR that was never merged in due to not signing the disclosure agreement (which I have)
📔 Objective
Adds support to all templates for specifying
nodeSelector
&tolerations
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes