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

fix: use external_url in alert source #635

Merged
merged 10 commits into from
Aug 23, 2024
Merged

Conversation

lucabello
Copy link
Contributor

Issue

Closes #581.
Closes canonical/alertmanager-k8s-operator#206.

Not sure why we were using the route_prefix from the self.external_url (from ingress), but adding it to the self.internal_url (which is always the FQDN).

Solution

Use the external URL instead of the internal one.

Screenshots

Before:
Screenshot-2024-08-14_10-23

After:
Screenshot-2024-08-14_10-39

Testing Instructions

juju deploy alertmanager-k8s --channel=latest/edge alertmanager
juju deploy prometheus-k8s --channel=latest/edge prometheus
juju deploy traefik-k8s --channel=latest/edge traefik

juju relate prometheus:alertmanager alertmanager
juju relate prometheus:metrics-endpoint alertmanager
# observe the watchdog alert's source link (internal url)
juju relate prometheus:ingress traefik
# wait for things to settle
# observe the same alert's source link again and try to click it!

Upgrade Notes

Anything that refreshes the Pebble plan will activate this change, so the upgrade should be seamless.

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

cough tests cough

@lucabello
Copy link
Contributor Author

A unit test has been fixed :)

I also skipped the tests we were XFail-ing, because they'll just waste time in integration tests without contributing :)

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

In principle the change is OK and given it's high prio I don't want to block it, but we're skipping a huge amount of tests for unclear reasons.
Can you explain what's going on and remove any tests that are no longer necessary?

tox.ini Show resolved Hide resolved
charmcraft.yaml Show resolved Hide resolved
@lucabello lucabello merged commit 030ff51 into main Aug 23, 2024
13 checks passed
@lucabello lucabello deleted the fix/wrong-alert-source-url branch August 23, 2024 07:30
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.

Internal url is rendered as the "source" in alerts Wrong Alert's source URL
3 participants