OCPBUGS-28246: fix Thanos ruler alert generator url #2267
Conversation
jan--f
commented
Feb 19, 2024
jan--f
commented
- I added CHANGELOG entry for this change.
- No user facing changes, so no entry in CHANGELOG was needed.
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
openshift-ci-robot
added
jira/valid-reference
|
@jan--f: This pull request references Jira Issue OCPBUGS-28246, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
approved
pkg/manifests/manifests.go
Outdated
| @@ -3340,8 +3339,11 @@ func (f *Factory) ThanosRulerCustomResource( | |||
| f.mountThanosRulerAlertmanagerSecrets(t) | |||
| f.injectThanosRulerAlertmanagerDigest(t, alertmanagerConfig) | |||
|
|
|||
| if queryURL != "" { | |||
| t.Spec.AlertQueryURL = queryURL | |||
| if f.consoleConfig != nil && f.consoleConfig.Status.ConsoleURL != "" { | |||
There was a problem hiding this comment.
Maybe we could refactor this under an util, we repeat the same logic in other places as well https://github.com/search?q=repo%3Aopenshift%2Fcluster-monitoring-operator%20consoleConfig.Status.ConsoleUR&type=code
There was a problem hiding this comment.
Will /monitoring be accessible/useful for non-admins as well?
(Simon mentioned sth similar at
cluster-monitoring-operator/pkg/manifests/manifests.go
Lines 480 to 481 in 1878d82
There was a problem hiding this comment.
Tbh I'm not sure what the current state of this is. Here I'd argue that even if a user doesn't have access to /monitoring, the back link to the console is still less wrong then linking to a UI that is not accessible for even admins.
There was a problem hiding this comment.
Maybe we could refactor this under an util, we repeat the same logic in other places as well https://github.com/search?q=repo%3Aopenshift%2Fcluster-monitoring-operator%20consoleConfig.Status.ConsoleUR&type=code
I thought about it, but given that we add the console URL different types and fields with different names I decided not to. Happy to be convinced with a good approach though...me failing to come up with something clever means very little.
There was a problem hiding this comment.
How about:
// we can even pass the factory...
func xxxExternalURL(f *ConsoleConfig, slug string) (string, error) {
if f != nil && f.Status.ConsoleURL != "" {
u, err = url.JoinPath(f.Status.ConsoleURL, path)
if err != nil {
return "", err
}
return url, nil
}
// "" is the zero value anyway
return "", nil
}
u, err := xxxExternalURL(f.consoleConfig, "monitoring")
if err != nil {
return nil, err
}
p.Spec.ExternalURL = u
wdyt?
There was a problem hiding this comment.
(cc @simonpasquier, if you know that
cluster-monitoring-operator/pkg/manifests/manifests.go
Lines 480 to 481 in 1878d82
There was a problem hiding this comment.
How about:
// we can even pass the factory... func xxxExternalURL(f *ConsoleConfig, slug string) (string, error) { ...wdyt?
I think we probably want to avoid setting the generator URLs to an empty string. So with a helper function we'd still have to
alertGeneratorURL, err := makeConsoleURL(f.consoleConfig, "monitoring")
if err != nil {
return nil, err
}
if alertGeneratorURL != "" {
t.Spec.AlertQueryURL = alertGeneratorURL
}
every time. Its not a deal breaker for me but I don't see the benefit (yet?). We still end up with 4 code places where we have to remember to set the console url just right.
There was a problem hiding this comment.
I think "" is the zero value, they'll get it anyway.
It was more of a nit. Your call ;)
There was a problem hiding this comment.
You are right of course. Added a commit accordingly.
|
/retest |
|
/test versions |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@jan--f: This pull request references Jira Issue OCPBUGS-28246, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
And use it to populate the various alert generator url fields. Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f, machine424 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-agnostic-operator |
|
/retest |
|
@jan--f: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit 5342dee
into
openshift:master
|
@jan--f: Jira Issue OCPBUGS-28246: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-28246 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build cluster-monitoring-operator-container-v4.16.0-202403052341.p0.g5342dee.assembly.stream.el9 for distgit cluster-monitoring-operator. |
|
Fix included in accepted release 4.16.0-0.nightly-2024-03-06-073110 |
