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

[bugfix] fix some bugs #172

Merged
merged 5 commits into from
Jul 13, 2023
Merged

Conversation

yandongxiao
Copy link
Collaborator

  1. reuse api.md and its script, because it is linked in StarRocks Doc
  2. delete namespace.yaml, otherwise helm install will failed.
  3. use resources not resource in values.yaml

1. reuse api.md and its script, because it is linked in StarRocks Doc
2. delete namespace.yaml, otherwise helm install will failed.
3. use resources not resource in values.yaml

Signed-off-by: yandongxiao <[email protected]>
Signed-off-by: yandongxiao <[email protected]>
@@ -274,7 +274,7 @@ starrocksCnSpec:
# scaleDown:
# selectPolicy: Disabled
# define resources requests and limits for cn pods.
resource:
resources:
Copy link
Collaborator

Choose a reason for hiding this comment

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

could cause compatible issue when the user upgrade from previous chart version to this, without updating its values.yaml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, This is a problem. But this misspell should be fixed.
So, we both support resources and resource?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, please. recommends the right spelling, but can still work if mis-spelled for now, possibly generating a warning message? can possibly remove the support after 2-3 releases in future.

{{- end }}
{{- if .Values.starrocksFESpec.resources }}
{{- toYaml .Values.starrocksFESpec.resources | nindent 4 }}
{{- else if .Values.starrocksFESpec.resource }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

does helm chart template support comments? shall add comments here to explain why if-else if here.

@kevincai kevincai merged commit 123aad3 into StarRocks:main Jul 13, 2023
4 checks passed
@yandongxiao yandongxiao added the bugfix fix something that does not work label Jul 14, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants