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

feat: add jifa helm chart #268

Closed
wants to merge 5 commits into from
Closed

Conversation

mark8s
Copy link
Contributor

@mark8s mark8s commented Mar 19, 2024

I found that jifa does not support helm deployment method, so I wrote one here. I've tested it and the service is available.

@y1yang0
Copy link
Contributor

y1yang0 commented Mar 19, 2024

This adds complexity but what's the gains?

@mark8s
Copy link
Contributor Author

mark8s commented Mar 19, 2024 via email

@mark8s
Copy link
Contributor Author

mark8s commented Mar 19, 2024 via email

@y1yang0
Copy link
Contributor

y1yang0 commented Mar 19, 2024

Please correct me if my understanding is wrong: This patch consolidates various configuration items scattered across different YAML files into a single YAML file(value.yaml?), and then utilizes these configuration items via referencing them in template YAML files. An obvious benefit I can see is that this makes it easier for users to configure various parameters for JIFA, and potentially, some platforms could visualize these parameters, among other things. However, on the flip side, a clear drawback is that in the future, each time we add a configuration item to JIFA's YAML, we would need to make corresponding changes to the Helm part. At a minimum, this would involve adding a new item to the values.yaml and then referencing it in the templates/*.yaml files. I believe that current and foreseeable configurations for JIFA will not be overly complex, and perhaps the attractiveness is not sufficient compared to the complexity introduced.

@mark8s
Copy link
Contributor Author

mark8s commented Mar 19, 2024 via email

@D-D-H
Copy link
Contributor

D-D-H commented Mar 19, 2024

@mark8s
I wanna know what storageClassName you are using.

The current value in cluster.yml is alibabacloud-cnfs-nas which is only available in Alibaba Cloud.

We should not hardcode it since it depends on the vendor, so I plan to delete it.

@mark8s
Copy link
Contributor Author

mark8s commented Mar 19, 2024 via email

@mark8s
Copy link
Contributor Author

mark8s commented Mar 20, 2024

@D-D-H I have added the license information, please take a look

@D-D-H
Copy link
Contributor

D-D-H commented Mar 20, 2024

My k8s cluster is set to the default sc (nfs), so I didn't set a specific sc. If a default storage class is set in the cluster, you do not need to set a specific value. K8s will select the default storage class by default.

Thanks for the clarification. I've removed the hard-coded StorageClass.

@D-D-H
Copy link
Contributor

D-D-H commented Mar 20, 2024

@D-D-H I have added the license information, please take a look

Okay. I'll take a full review these two days.

@mark8s mark8s closed this Mar 25, 2024
@mark8s mark8s deleted the feat/helm branch March 25, 2024 01:28
@D-D-H
Copy link
Contributor

D-D-H commented Mar 25, 2024

@mark8s

Are you still working on this?

@mark8s mark8s mentioned this pull request Mar 25, 2024
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.

3 participants