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(victoriametrics): add VictoriaMetrics database #20158

Merged
merged 2 commits into from Apr 8, 2024

Conversation

xunleii
Copy link
Contributor

@xunleii xunleii commented Apr 2, 2024

Description

Hi,

This PR adds Victoria Metrics database.

NOTE: it is installed only as an application, not as a possible replacement for Prometheus integration with TrueCharts (related to #6185).

⚙️ Type of change

  • ⚙️ Feature/App addition
  • 🪛 Bugfix
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 🔃 Refactor of current code

🧪 How Has This Been Tested?

Currently not tested (only with Helm template to see the generated YAML)

📃 Notes:

✔️ Checklist:

  • ⚖️ My code follows the style guidelines of this project
  • 👀 I have performed a self-review of my own code
  • #️⃣ I have commented my code, particularly in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ⚠️ My changes generate no new warnings
  • 🧪 I have added tests to this description that prove my fix is effective or that my feature works
  • ⬆️ I increased versions for any altered app according to semantic versioning
  • I made sure the title starts with feat(chart-name):, fix(chart-name): or chore(chart-name):

➕ App addition

If this PR is an app addition please make sure you have done the following.

  • 🖼️ I have added an icon in the Chart's root directory called icon.png

Please don't blindly check all the boxes. Read them and only check those that apply.
Those checkboxes are there for the reviewer to see what is this all about and
the status of this PR with a quick glance.

Signed-off-by: Alexandre Nicolaie <xunleii@users.noreply.github.com>
@xunleii xunleii requested review from Ornias1993 and a team as code owners April 2, 2024 19:43
@CLAassistant
Copy link

CLAassistant commented Apr 2, 2024

CLA assistant check
All committers have signed the CLA.

@Ornias1993
Copy link
Member

I think we've already veto'ed this internally before.
We don't want to give any impression that unsupported databases are actually supported in any way.

Copy link
Member

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

.

@xunleii
Copy link
Contributor Author

xunleii commented Apr 3, 2024

I think we've already veto'ed this internally before.
We don't want to give any impression that unsupported databases are actually supported in any way.

Sorry, I thought the rejection was about the integration part with Prometheus and not the fact that it's a database.
I saw this application as being of the same type as Clickhouse or Redis, which are therefore considered supported.

Is there anything I can do to make this DB supported?
I'd also suggest putting in the documentation (like https://truecharts.org/manual/development/contributing#review-guidelines ?) that the DBs application must be validated upstream by you before launching development (just after validating this PR, I was going to do the same for InfluxDB), to avoid this happening again.

@xunleii xunleii requested a review from Ornias1993 April 3, 2024 21:57
@Ornias1993
Copy link
Member

Ornias1993 commented Apr 6, 2024

I saw this application as being of the same type as Clickhouse or Redis, which are therefore considered supported.

It is, we also don't support clickhouse or redis alternatives.
For dependencies, or things that might be percieved as a dependency, such as databases we only allow one of a kind.

For example:
InfluxDB has also been denied for the same reason.

https://truecharts.org/manual/development/contributing#review-guidelines

That documentation is outdated at best. You're always free to update any documentation as you see fit.

But even so, all contributions are always depending on maintainer approval.
We might or might not have internal reasons not to approve or merge any PR, that are not listed in public documentation.

that the DBs application must be validated upstream by you before launching development

It has nothing to do with validating upstream (which would be the folks at victoria metrics in this case), it has to do with getting maintainer permission here. I think you're confusing upstream with getting greenlight here, upstream means the originating project (aka container source).

just after validating this PR, I was going to do the same for InfluxDB

I'm open to discuss allowing this and or influxdb next month, but this needs to be discussed internally before merge.
I'm not saying "no", i've not closed this PR yet.

I'm just blocking it by putting a review block on it, so lower-level staff doesn't merge it by accident.

@xunleii
Copy link
Contributor Author

xunleii commented Apr 7, 2024

It is, we also don't support clickhouse or redis alternatives.
For dependencies, or things that might be percieved as a dependency, such as databases we only allow one of a kind.

Okay, I see now. Because Clickhouse and Redis was in the "stable" folder of this repository, I thought it was something that we can just install like others charts. As they are dependencies, it now makes sense.

It has nothing to do with validating upstream (which would be the folks at victoria metrics in this case), it has to do with getting maintainer permission here. I think you're confusing upstream with getting greenlight here, upstream means the originating project (aka container source).

Yes, I also used the wrong term for this case ; for me, the "upstream" term refers to the "repository where the fork comes from" and not the "upstream project". My bad for that misunderstanding.

I'm open to discuss allowing this and or influxdb next month, but this needs to be discussed internally before merge.
I'm not saying "no", i've not closed this PR yet.

I'm just blocking it by putting a review block on it, so lower-level staff doesn't merge it by accident.

So thank you for taking the time to clarify things on this PR. Please don't hesitate to ping me if there are any changes in the future that you'd like to have VM in your TrueNAS charts / catalog.

@Ornias1993
Copy link
Member

Okay, I see now. Because Clickhouse and Redis was in the "stable" folder of this repository, I thought it was something that we can just install like others charts. As they are dependencies, it now makes sense.

Uhmm, you can technically install them like any other chart, no one said otherwise.
Or at least: you should be able to.

Copy link
Member

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

If you add a docs page named "warning", clealy highlighting:

  • It's offered as stand-alone chart only, it cannot be connected to any other chart offered by TrueCharts
  • We do not support connecting this with any other charts offered by TrueCharts
  • It might break other charts offered by TrueCharts, use at your own risk
  • No support will be offered anymore for users running any form of Metrics (prometheus or otherwise) with this chart installed.
  • We advice heavily against using this with any other TrueCharts chart

With that in a docs page included, we can merge this.


I think with those warnings, you also will start to see why it's not really usefull including it at-all.

@xunleii
Copy link
Contributor Author

xunleii commented Apr 7, 2024

If you add a docs page named "warning", clealy highlighting:

  • It's offered as stand-alone chart only, it cannot be connected to any other chart offered by TrueCharts
  • We do not support connecting this with any other charts offered by TrueCharts
  • It might break other charts offered by TrueCharts, use at your own risk
  • No support will be offered anymore for users running any form of Metrics (prometheus or otherwise) with this chart installed.
  • We advice heavily against using this with any other TrueCharts chart

With that in a docs page included, we can merge this.

I think with those warnings, you also will start to see why it's not really usefull including it at-all.

Yes, with all these warnings, it seems to be something external that has been integrated into TrueCharts and, in my opinion, looks "weird".

What I can suggest, for now, is to keep this PR open; if someone who is interested in this can put a thumbs up on this PR and, if there are several requests for it, I will see about adding the warnings as you suggest.

NOTE: on my side, I'm going to create a dedicated catalog for myself with VictoriaMetrics and probably some integration between VictoriaMetrics and Prometheus Operator.

@xunleii xunleii requested a review from Ornias1993 April 7, 2024 10:16
@Ornias1993
Copy link
Member

Yes, with all these warnings, it seems to be something external that has been integrated into TrueCharts and, in my opinion, looks "weird".

But it is something that is not integrated into our platform at all.
Its simply not something we support at all, which is fine but needs to be made clear to users.

if someone who is interested in this can put a thumbs up on this PR and, if there are several requests for it, I will see about adding the warnings as you suggest.

I'm going to be strict here:
PR's are not for voting, if you want it merged: add the requested docs.
Otherwise it needs to be closed

@xunleii
Copy link
Contributor Author

xunleii commented Apr 7, 2024

But it is something that is not integrated into our platform at all.
Its simply not something we support at all, which is fine but needs to be made clear to users.

I'm sorry if my message was misunderstood; I agreed with you and that's what I meant: there are no plans to integrate it into TrueCharts (I know that) and for me, having these warnings means it doesn't really belong in this repo (it looks more like a workarround than something made and manage by this repo).

I'm going to be strict here:
PR's are not for voting, if you want it merged: add the requested docs.
Otherwise it needs to be closed

Okay, so I prefer to close this PR.


Thank you for your patience and sorry if I misspoke a few times, English is not my cup of tea 😅

@xunleii xunleii closed this Apr 7, 2024
@Ornias1993 Ornias1993 reopened this Apr 8, 2024
@Ornias1993 Ornias1993 merged commit 6064a8f into truecharts:master Apr 8, 2024
17 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