Skip to content

[BugFix] Allow to remove some resources limit, like cpu #426

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

Conversation

yandongxiao
Copy link
Collaborator

@yandongxiao yandongxiao commented Jan 30, 2024

Related Issue(s)

Fixes: #422

Checklist

For helm chart, please complete the following checklist:

  • make sure you have updated the values.yaml
    file of starrocks chart.
  • In scripts directory, run bash create-parent-chart-values.sh to update the values.yaml file of the parent
    chart( kube-starrocks chart).

{{- else if .Values.starrocksCnSpec.resource }}
{{- include "starrockscluster.cn.resources" . | nindent 4 }}
{{- end }}
{{- if .Values.starrocksCnSpec.resource }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should merge the two if statements into a single if..end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

committed

@yandongxiao yandongxiao force-pushed the bugfix/make-it-possible-to-remove-resource-limit branch from f871641 to 8ac069c Compare January 30, 2024 05:45
{{- define "starrockscluster.cn.resources" -}}
resources:
requests:
{{- range $key, $value := .Values.starrocksCnSpec.resources.requests }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not directly toYaml here since there is no specific processing to the key/value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No special reason, just keep same with limits.
I will try to change to toYaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

committed

@yandongxiao yandongxiao force-pushed the bugfix/make-it-possible-to-remove-resource-limit branch from 8ac069c to 65328f7 Compare January 30, 2024 05:59
@yandongxiao yandongxiao merged commit b5bca9c into StarRocks:main Jan 30, 2024
@yandongxiao yandongxiao added v1.9.2 bugfix fix something that does not work labels Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fix something that does not work v1.9.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the resource limit in the default value
3 participants