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 Helm Chart for the OpenSearch cluster CR #478

Merged

Conversation

IshaGirdhar
Copy link
Contributor

@IshaGirdhar IshaGirdhar commented Apr 4, 2023

This fixes #448
PR to add Helm Chart to install OpenSearch cluster CR.

Copy link
Collaborator

@idanl21 idanl21 left a comment

Choose a reason for hiding this comment

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

Installed and it working great! didn't tested all values fields, but the example is working great.
it is big improvment, im still thinking about how to add some test for that so it will not break in the future.
Also, please add here https://github.com/Opster/opensearch-k8s-operator/blob/85140dfba76aee74f57867997f89d87a199eaf23/MAINTAINERS.md#release-process also update the chart as part of the release process, in case that the CRD as changed

charts/opensearch-cluster/README.md Outdated Show resolved Hide resolved
charts/opensearch-cluster/README.md Outdated Show resolved Hide resolved
@swoehrl-mw
Copy link
Collaborator

Hi @IshaGirdhar.
IMO this is the wrong approach here.

What would be beneficial is adding a cluster spec to the helm chart of the operator itself (hidden under a by default disabled value), for those who just want to install one cluster and don't want to deploy the operator and the cluster spec separately (for example the prometheus-operator does something similar where you get the operator + 1 prometheus stack by installing the chart and can always create additional stacks by deploying additional CRs).

But a separate helm chart with only a cluster spec is IMO not very helpful. If someone is deploying several opensearch clusters (and is doing this as part of some larger deployment) then they most likely already have either mechanisms for deploying kubernetes yamls (so they don't need the overhead of the helm chart) or have their own custom helm chart around that combines such single yamls into one deployment (my personally preferred approach).

So IMO the helm chart as you have written it is not something we should add to the operator offering.

@IshaGirdhar
Copy link
Contributor Author

Hi @IshaGirdhar. IMO this is the wrong approach here.

What would be beneficial is adding a cluster spec to the helm chart of the operator itself (hidden under a by default disabled value), for those who just want to install one cluster and don't want to deploy the operator and the cluster spec separately (for example the prometheus-operator does something similar where you get the operator + 1 prometheus stack by installing the chart and can always create additional stacks by deploying additional CRs).

But a separate helm chart with only a cluster spec is IMO not very helpful. If someone is deploying several opensearch clusters (and is doing this as part of some larger deployment) then they most likely already have either mechanisms for deploying kubernetes yamls (so they don't need the overhead of the helm chart) or have their own custom helm chart around that combines such single yamls into one deployment (my personally preferred approach).

So IMO the helm chart as you have written it is not something we should add to the operator offering.

@swoehrl-mw What you say makes sense to me and that also supports our use case as well. The only reason for taking this approach was to support multiple clusters using helm. Sure I can modify my existing PR or open a new PR for what you proposed.

@IshaGirdhar
Copy link
Contributor Author

@swoehrl-mw On second thoughts, this solution seems to be fine as well, as this supports what the operator supports(multiple clusters), doesn't have any technical challenges though yeah users may have their own automated ways to create multiple clusters but that doesn't stop them from using Helm to do the same.

@IshaGirdhar IshaGirdhar changed the title WIP: Add Helm Chart for the OpenSearch cluster CR Add Helm Chart for the OpenSearch cluster CR May 2, 2023
@idanl21
Copy link
Collaborator

idanl21 commented May 4, 2023

Hey @swoehrl-mw @prudhvigodithi
What are you saying about that PR ?

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Left some specific comments.
Also:

  • Extend the workflows to release this chart (and update the versions)
  • Extend the developer guide that additions to the CRDs must also be reflected in this chart so they stay in sync
  • Extend the description in MAINTAINERS.md
  • Extend the functionaltests so that they also try to install this chart to see if it is valid

@idanl21 Truthfully I'm still not convinced this chart is a good idea. I believe most people that want to do everything with helm and have a proper deployment structure and process will rather either build a custom chart or have such a chart already where they can include the CR. But if other people see value in this I'll not gonna block it.

charts/opensearch-cluster/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch-cluster/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch-cluster/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch-cluster/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch-cluster/README.md Outdated Show resolved Hide resolved
@IshaGirdhar IshaGirdhar force-pushed the igirdhar/helm-chart-os-cluster branch from 57da512 to b1d4f2a Compare May 15, 2023 09:29
charts/opensearch-cluster/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch-cluster/README.md Outdated Show resolved Hide resolved
charts/opensearch-cluster/README.md Outdated Show resolved Hide resolved
@IshaGirdhar
Copy link
Contributor Author

@swoehrl-mw Please check. I have addresses all review comments.

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Two nitpicks, looks good otherwise.

docs/userguide/main.md Outdated Show resolved Hide resolved
docs/userguide/cluster-chart.md Outdated Show resolved Hide resolved
@IshaGirdhar IshaGirdhar force-pushed the igirdhar/helm-chart-os-cluster branch from 0a0c28a to 70db5da Compare June 2, 2023 08:45
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
Signed-off-by: Isha Girdhar <isha.girdhar@oracle.com>
@IshaGirdhar IshaGirdhar force-pushed the igirdhar/helm-chart-os-cluster branch from 70db5da to b64c583 Compare June 2, 2023 09:10
@swoehrl-mw swoehrl-mw merged commit 72d46e2 into opensearch-project:main Jun 2, 2023
5 checks passed
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.

None yet

3 participants