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

Enable Multi Cluster alerts by default #2099

Merged
merged 1 commit into from
May 22, 2023

Conversation

jkroepke
Copy link
Contributor

@jkroepke jkroepke commented May 3, 2023

Description

Fixes #2056

Type of change

What type of changes does your code introduce to the kube-prometheus? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. Later this will be copied to the changelog file.

Enable Multi Cluster alerts by default

@jkroepke
Copy link
Contributor Author

@philipgough do you have some time for a review here?

@philipgough
Copy link
Contributor

@jkroepke - kicked off the pipeline and it fails, can you rebase?

@philipgough philipgough self-assigned this May 17, 2023
@jkroepke
Copy link
Contributor Author

@philipgough rebased.

@philipgough
Copy link
Contributor

@simonpasquier - would you mind taking a look here? I think this makes sense to me but I wan't to see how it looks from your side?

@jkroepke
Copy link
Contributor Author

I would appreciate a review before a next merge conflict appears :)

@simonpasquier
Copy link
Contributor

No objection from my side. Ideally it should be consistent and there should be a single config parameter in kube-prometheus while right now, it seems that some mixins support toggling multi-cluster on/off while for others, it's hard-coded.

Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

lgtm

@philipgough
Copy link
Contributor

Thanks Simon, followed up with #2120 based on your comment

@jkroepke jkroepke deleted the multi-cluster branch May 22, 2023 15:33
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.

Add support for multi cluster alert rule queries
3 participants