-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
operator: Add alertingrule tenant id label for all rules #8748
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits. lgtm otherwise.
operator/bundle/openshift/manifests/loki-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
operator/bundle/openshift/manifests/loki-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
operator/bundle/openshift/manifests/loki-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
1483748
to
3a603c0
Compare
3a603c0
to
c17bcee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
3cb005f
to
d9263a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review
rule.Labels = map[string]string{} | ||
} | ||
|
||
rule.Labels[tenantLabel] = a.Spec.TenantID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought I had posted this comment last week already, but apparently I never clicked "publish" ...
It looks like we need to do a DeepCopy
somewhere, because we are modifying the Groups
(dirtying the lister cache). These are all pointer-types, so not only the one in alertingRuleSpec
is modified but also the original in AlertingRule
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you posted this on slack, I started working it out and stashed it. It got forgotten :(
What this PR does / why we need it:
The following PR adds a AlertingRule defaulting webhook is to populate the tenant ID to all rules to enable managing Alertmanager notifications per tenant.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updateddocs/sources/upgrading/_index.md