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.

Sign up for GitHub

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

Jump to bottom

[victoria-metrics-k8s-stack] Prometheus Operator CRDs (#341) #1022

Merged
merged 2 commits into from
May 16, 2024
Merged

[victoria-metrics-k8s-stack] Prometheus Operator CRDs (#341) #1022

merged 2 commits into from
May 16, 2024

Conversation

b17k0
Copy link
Contributor

@b17k0 b17k0 commented May 14, 2024

Hi! Small patch for #341

@b17k0 b17k0 requested review from Amper, zekker6 and Haleygo as code owners May 14, 2024 14:05
@b17k0 b17k0 changed the title [victoria-metrics-k8s-stack] Prometheus Operator CRDs (https://github.com/VictoriaMetrics/helm-charts/issues/341) [victoria-metrics-k8s-stack] Prometheus Operator CRDs (#341) May 14, 2024
@Haleygo
Copy link
Contributor

Haleygo commented May 16, 2024 •
edited
Loading

I have one concern though, CRDs in prometheus-operator-crds are under /crds/templates, so they are treated as regular templates and will be managed by Helm. That means prometheus CRDs also will be deleted when uninstall VM release[with prometheus-operator-crds.enabled], and it could break all the prometheus CRs that user has, which is not good for VM-k8s-stack IMO.

@b17k0
Copy link
Contributor Author

b17k0 commented May 16, 2024

@Haleygo Yes, but this is an optional feature. It is always possible to make prometheus-operator-crds independent, it sets prometheus-operator-crds.enabled=false and sets prometheus-operator-crds as a separate chart.
But I think there will not be many such users. Most users of victoria-metrics-k8s-stack appreciate being able to set everything with one chart.

@Haleygo
Copy link
Contributor

Haleygo commented May 16, 2024

Most users of victoria-metrics-k8s-stack appreciate being able to set everything with one chart.

Thanks, I'm ok with the default value prometheus-operator-crds.enabled=false.
Do you have second opinion? @zekker6 @f41gh7 @Amper

zekker6
zekker6 approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@zekker6 zekker6 left a comment

Choose a reason for hiding this comment

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

LGTM

f41gh7
f41gh7 approved these changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@f41gh7 f41gh7 left a comment

Choose a reason for hiding this comment

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

LGTM

@zekker6 zekker6 merged commit 37a9d72 into VictoriaMetrics:master May 16, 2024
7 of 8 checks passed
zekker6 added a commit that referenced this pull request May 20, 2024
@zekker6
charts/victoria-metrics-k8s-stack: sync deps
b48953d 
Sync deps after #1022
@zekker6 zekker6 mentioned this pull request May 20, 2024
Merged
zekker6 added a commit that referenced this pull request May 20, 2024
@zekker6
charts/victoria-metrics-k8s-stack: sync deps (#1030)
7be0cca 
Sync deps after #1022
@zekker6 zekker6 mentioned this pull request May 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f41gh7 f41gh7 f41gh7 approved these changes

@zekker6 zekker6 zekker6 approved these changes

@Amper Amper Awaiting requested review from Amper

@Haleygo Haleygo Awaiting requested review from Haleygo Haleygo is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@b17k0 @Haleygo @zekker6 @f41gh7