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

[kong] add nodeSelector to migration Jobs #238

Merged
merged 2 commits into from
Dec 14, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Dec 11, 2020

What this PR does / why we need it:

The nodeSelector value was previously only applied to the main Deployment. It should be applied to migration Jobs also, since they run Kong also, and have the same platform requirements.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes FTI-2152

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not main
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@@ -58,6 +58,10 @@ spec:
{{- include "kong.volumeMounts" . | nindent 8 }}
securityContext:
{{- include "kong.podsecuritycontext" . | nindent 8 }}
{{- if .Values.nodeSelector }}
nodeSelector:
{{ toYaml .Values.nodeSelector | indent 8 }}
Copy link
Member

Choose a reason for hiding this comment

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

Before this PR, all 3 edited files prefer nindent (with {{-) to indenting - this PR appears to be the first introduction of indent.

Is there a rationale for breaking consistency? If not, could we do:

Suggested change
{{ toYaml .Values.nodeSelector | indent 8 }}
{{- toYaml .Values.nodeSelector | nindent 8 }}

throughout this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. Copy-pasta from the deployment template; newline-chomping convention isn't consistent throughout the templates.

Copy link
Member

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

Conditional LGTM, assuming that the changes have been tested to work.

@rainest
Copy link
Contributor Author

rainest commented Dec 14, 2020

assuming that the changes have been tested to work.

It's manual since we don't have an exhaustive test values.yaml with all non-default settings. I set it to an example value to confirm that the indentation is correct/didn't result in invalid Job resources rendering.

@rainest rainest merged commit 6ec8e6d into next Dec 14, 2020
@rainest rainest deleted the fix/job-nodeselector branch December 14, 2020 23:10
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.

None yet

2 participants