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

Support persistent volumes in Cosmos chains #255

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Inkvi
Copy link
Collaborator

@Inkvi Inkvi commented Sep 29, 2023

No description provided.

@Inkvi Inkvi requested a review from Anmol1696 September 29, 2023 23:42
@@ -2,6 +2,8 @@ chains:
- name: osmosis-1
type: osmosis
numValidators: 1
storageClassName: standard-rwo
storage: 1Gi
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we update this to

storage:
  className: standard-rwo
  size: 1Gi

Might make more sense for the users. I would like to avoid exposing all the k8s specific naming directly to the users (if it can be avoided and made simpler).

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 imagine that this is mostly needed for production use case. At least, that's when we use it. With this assumption, I believe that a Starship user knows what they are doing and familiar with k8s concepts and hence they want to have better controls. Otherwise, Starship needs to expose the same concepts but with Starship alternatives. That doesn't sounds like a good UX to me.

If you have better ideas at how to allow fine controls, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, we dont have to change the names, but if we could still move it into storage, it will make the happy path to be less cluttered.

{{- if hasKey $chain "storage" }}
- name: init-chmod-data
image: busybox
command: ['sh', '-c', 'chmod -R 0777 {{ $chain.home }}']
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm maybe there is another way to do this. Will have to look into this.

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 depends on what user runs the containers. If you know a better way, I would gladly implement it.

@Anmol1696
Copy link
Collaborator

@Inkvi could we maybe move the same directives inside something like 'storage', we can still keep the names same.

The idea is to be able to make helm charts more modular, a flat hierarchy will get confusing to expand.

Helm charts usually also do this, have a look at 'persistence' directive in this chart: https://github.com/bitnami/charts/blob/main/bitnami/cassandra/values.yaml

Also for flags, the norm is to use '.enabled: true'.

@Anmol1696
Copy link
Collaborator

I can take care of it too, as part of beautification of the helm charts itself as well, if you want.

We can merge this as a feature, and then i can update it, but just wanted to let you know how i am thinking of this.

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