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

Add local_storage_size option in values #594

Merged
merged 2 commits into from
Jul 12, 2023
Merged

Conversation

puppetninja
Copy link
Collaborator

Some cloud provider have minimum pvc size requirement, like Aliyun, this option would also tezos-k8s to be deployed under such scenarios.

Some cloud provider have minimum pvc size requirement, like Aliyun,
this option would also tezos-k8s to be deployed under such scenarios.
Comment on lines 89 to 92
storage: {{ default "1Gi" $.node_vals.local_storage_size }}
{{- else }}
storage: {{ default "15Gi" $.node_vals.storage_size }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to introduce a brand new field here. You can use logic to determine the default size and just use storage_size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be confusing, to configure pvcs for different purposes with same helm value, they even have different pvc names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i see what you are saying. Maybe we rename local_storage_size then because that sounds like the size of the local temp memory of the machine which isn't persistent and isn't what is being configured. Maybe local_storage_pv_size?

Copy link
Collaborator

@harryttd harryttd left a comment

Choose a reason for hiding this comment

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

Thank you!

@puppetninja puppetninja merged commit a53fd7f into master Jul 12, 2023
20 checks passed
@puppetninja
Copy link
Collaborator Author

I rebased it instead of squashing my commits, sorry for that

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.

2 participants