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

helm: use toYaml for discovery nodeAffinity #13931

Merged
merged 1 commit into from Mar 15, 2024

Conversation

hhk7734
Copy link
Contributor

@hhk7734 hhk7734 commented Mar 14, 2024

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I see this will now have the same format that the CSI settings are already using, so it makes sense.

Could you just squash to a single commit? Or else the second commit needs a description message. A single commit is fine since it's the same description.

this commit allow user to use requiredDuringSchedulingIgnoredDuringExecution map

Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
@hhk7734
Copy link
Contributor Author

hhk7734 commented Mar 15, 2024

@travisn

I squashed it into a single commit :)

@travisn travisn merged commit a4ccb81 into rook:master Mar 15, 2024
51 checks passed
mergify bot added a commit that referenced this pull request Mar 15, 2024
helm: use toYaml for discovery nodeAffinity (backport #13931)
@@ -68,7 +68,7 @@ spec:
{{- end }}
{{- if .Values.discover.nodeAffinity }}
- name: DISCOVER_AGENT_NODE_AFFINITY
value: {{ .Values.discover.nodeAffinity }}
value: {{ toYaml .Values.discover.nodeAffinity | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

why quote @hhk7734 ?

Copy link
Contributor Author

@hhk7734 hhk7734 Mar 15, 2024

Choose a reason for hiding this comment

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

an error occurs if there is no quote when using dict type yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without quote

        - name: DISCOVER_AGENT_NODE_AFFINITY
          value: |
            {{- toYaml .Values.discover.nodeAffinity | nindent 12 }}

Copy link
Member

Choose a reason for hiding this comment

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

yes without quote make more sense to me, not sure how it is nindent if just specify the quote keyword, do you have any document that explains it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://helm.sh/docs/chart_template_guide/functions_and_pipelines/

Using | quote in env[].value is very common, so I cannot get the reason to replace to nindent.

Even you can see using quote above this line.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants