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

Update configmap -completed version #19

Closed

Conversation

seanfariacustomink
Copy link

What this PR does / why we need it:

for MySQL and PostgreSQL

Which issue this PR fixes

lack of mysql

 for MySQL and PostgreSQL
@sa-ChristianAnton
Copy link
Contributor

I am actually not too happy with this PR.

  • the commits are not commented at all, not even the PR has a title saying what it is actually all about
  • no example added for how to use it (charts/zabbix/docs/example/)
  • No documentation updated, no version bumped (see CONTRIBUTING.md)

I don't see that this PR has been created with some "love" spent to make it good.

Generally, I am not yet sure wether we "want to" support MySQL in this Chart at all...? It's quite some extra effort to maintain.... opionions please. .... @aeciopires what do you think?

@aeciopires
Copy link
Member

Hello @seanfariacustomink !

Thank you for your contribution with this helm chart. You've identified a need, opened an issue, and are now submitting a PR. This is very appreciated.

But @sa-ChristianAnton is right. With the merge of this PR, we will have triplicate work with each release of new versions of the helm chart to test the changes in three databases (MySQL, MariaDB and PostgreSQL). For that reason, we'd like to see a few more things in this PR to make our jobs easier and give others clarity on what a great feature this helm chart is getting.

What still needs to be done in this PR is the following:

  • In Chart.yaml file, change version from 3.4.3 to 4.0.0.
  • In values.yaml and docs/example/kind/values.yaml files, we need of new parameters with same documentation quality for to show other people how to deploy the Zabbix using MySQL 5.x, 8.x, MariaDB 5.x, 8.x or PostgreSQL 14.x, but with preference to PostgreSQL because this helm chart offer support this database since begin.
  • In README.md and docs/example/README.md files, we need the documentation for to show other people how to install this helm chart using MySQL 5.x, 8.x, MariaDB 5.x, 8.x or PostgreSQL 14.x. We need too put the official charts of MySQL and MariaDB as a optional dependencies.
  • In the files that has manipulation of database, we need code for customize the integration with MySQL and MariaDB installed by official charts.

Could you help us with this? Are you willing to help @sa-ChristianAnton and I with basic tests of running Zabbix every launch of the helm chart?

Perhaps put a disclaimer that this helm chart is primarily tested with PostgreSQL and that problems with other databases may occur and will be fixed on demand. What do you think @sa-ChristianAnton? With these changes does it look good to you?

@sa-ChristianAnton
Copy link
Contributor

I fully agree with @aeciopires with the following additional comments:

  • I think it would be sufficient to just support MySQL 8 (at least when it comes to deploy the DB pod ourselves) and to document that this is the only tested version, when also external DBs with other versions (but supported by Zabbix) would work
  • Because of that, it is probably enough to deploy the MySQL the same way we do the Postgresql now, without the need to pull in other helm charts as dependencies.

I am looking forward to this "project", as it is one, not just a little change...

@aeciopires
Copy link
Member

The comment of @seanfariacustomink in the issue #18 is interesting. Maybe we can work together indicating the chart https://github.com/dj-wasabi/helm-zabbix#install-the-helm-chart for people who need to use Zabbix with MySQL.

What do you think @sa-ChristianAnton and @seanfariacustomink?

@sa-ChristianAnton
Copy link
Contributor

The comment of @seanfariacustomink in the issue #18 is interesting. Maybe we can work together indicating the chart https://github.com/dj-wasabi/helm-zabbix#install-the-helm-chart for people who need to use Zabbix with MySQL.

What do you think @sa-ChristianAnton and @seanfariacustomink?

Not sure, as the referenced chart is not being updated for quite some time. I would rather prefer extending this project.

Maybe we should implement automated tests for deployments, upgrades, etc. of Zabbix instances with this helm chart in different scenarios? Like "with HA, with Postgres, with TimescaleDB enabled, with MySQL instead", ....? This would lower the burden of manual testing after every little change and we can support MySQL without problems.

Does Github offer this kind of possibilities with no cost?

@aeciopires
Copy link
Member

You're right @sa-ChristianAnton, we can extend this project...

I'll look into the cost of GitHub Actions.

Implementing the automated tests for this helm chart will be a great learning challenge. I've never done this before and I'm willing to learn. But for now we will wait for the response from @seanfariacustomink

@sa-ChristianAnton
Copy link
Contributor

I'll look into the cost of GitHub Actions.

I could imagine that providing a Kubernetes cluster on which to deploy things is not something Github Actions will provide for free. In that case I can provide one running in my home lab, or somewhere else, so that only executing the tests would have to be triggered from Github somehow.

@seanfariacustomink
Copy link
Author

Hi all,

After playing with this over the Christmas break I now know this entire PR will have to change.
I don't think it is required to support MYSQL DB within helm but rather the ability to point to a (MySQL, Maria, etc) database and disable Postgresql DB creation within the cluster. This approach would be less work and be perfect for anyone with an existing Zabbix application to move to the helm version. I will most likely create another PR for that.

@aeciopires aeciopires added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request break change A change in one part of a software system that potentially causes other components to fail labels May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break change A change in one part of a software system that potentially causes other components to fail bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants