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

Provide helm chart deploy method #707

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Provide helm chart deploy method #707

wants to merge 16 commits into from

Conversation

Eroyi
Copy link

@Eroyi Eroyi commented Oct 29, 2024

Using helm chart to deploy:

  • Mordred
  • Nginx
  • Sortinghat
  • Sortinghat Worker
  • Opensearch
  • Opensearch Dashboard

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

This looks awesome. Thanks for the PR.

However, pardon my ignorance about Kubernetes. Does it mean we can remove the other files that we have to deploy on kubernetes? Probably doesn't make sense to use different methods, specially when Helm is used everywhere.

Could you also update the README file to explain a little bit more about how to configure and deploy it using helm.

Also, all files don't end on an empty line. Please update them.

@Eroyi
Copy link
Author

Eroyi commented Oct 31, 2024

Does it mean we can remove the other files that we have to deploy on kubernetes?

Absolutely, you may remove those deployment.yaml files since the helm chart is now available for Kubernetes deployment.
But I must remind that helm charts, especially nested charts like this, are highly abstracted compared to the loose yaml file, which means it's more tricky to debug and hot-wire things.
I'll maintain this as long as I can, but highly recommend you get a helm chart specialist rather than rely on the community.

Could you also update the README file to explain a little bit more about how to configure and deploy it using helm.
Also, all files don't end on an empty line. Please update them.

Sure, will do.

@Eroyi Eroyi requested a review from sduenas November 7, 2024 08:21
@sduenas
Copy link
Member

sduenas commented Nov 7, 2024

@Eroyi tell me when you think the PR is ready to review it.

Eroyi added 12 commits November 7, 2024 18:09
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
- reconstruct the helm chart structures, now variables can be defined and referencing across all sub chart
- add template file to handle variables
- fix format issue, all files end with empty line.
- (WIP) integrate with bitnami's redis and mariadb chart.
- (TODO) README.md
- (ISSUE) variables in templates/_ports.tpl cannot be obtain correctly from values.yaml. All variables are fixed for now.

Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
- reconstruct the helm chart structures, now variables can be defined and referencing across all sub chart
- add template file to handle variables
- fix format issue, all files end with empty line.
- (WIP) integrate with bitnami's redis and mariadb chart.
- (ISSUE) variables in templates/_ports.tpl cannot be obtained correctly from values.yaml. All variables are fixed for now.

Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Signed-off-by: Arie <[email protected]>
Signed-off-by: root <[email protected]>
Eroyi added 4 commits November 7, 2024 18:21
- instruction on how to replace database connection strings
- instruction on how to replace opensearch, opensearch dashboard connection strings
- instruction on how to import dashboard

Signed-off-by: Arie <[email protected]>
@Eroyi
Copy link
Author

Eroyi commented Nov 8, 2024

@Eroyi tell me when you think the PR is ready to review it.

All of a sudden I failed to clone any GitHub repos from my Ubuntu, so I had to do things through the browser on github.com and Windows PC. Apologise for the messy commits.

Anyway I scrubbed all the Windows format toxics, LGTM now, please review.

This chart is tested in our Kubernetes cluster (with mariadb and redis installed separately).

@sduenas
Copy link
Member

sduenas commented Dec 3, 2024

@Eroyi tell me when you think the PR is ready to review it.

All of a sudden I failed to clone any GitHub repos from my Ubuntu, so I had to do things through the browser on github.com and Windows PC. Apologise for the messy commits.

Anyway I scrubbed all the Windows format toxics, LGTM now, please review.

This chart is tested in our Kubernetes cluster (with mariadb and redis installed separately).

I haven't merged this PR yet because I talked the other day to some people interested on checking it too. Let's wait some days and if we haven't heard anything, we'll merge it.

@Eroyi
Copy link
Author

Eroyi commented Dec 6, 2024

@Eroyi tell me when you think the PR is ready to review it.

All of a sudden I failed to clone any GitHub repos from my Ubuntu, so I had to do things through the browser on github.com and Windows PC. Apologise for the messy commits.
Anyway I scrubbed all the Windows format toxics, LGTM now, please review.
This chart is tested in our Kubernetes cluster (with mariadb and redis installed separately).

I haven't merged this PR yet because I talked the other day to some people interested on checking it too. Let's wait some days and if we haven't heard anything, we'll merge it.

@sduenas thank you for your following upp. Yeah i noticed that issue you mentioned, I haven't seen any replied too. Speak of that I am highly recommend you or your team give a try on this helm chart before merging, in case of anything doesn't work as expected (or any ambiguous in readme / instruction), there I can fix it asap before it affetcs the users.

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