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

Alerting: Refactor the ImageStore/Provider to provide image URL/bytes #67693

Merged
merged 18 commits into from
May 30, 2023
Merged

Alerting: Refactor the ImageStore/Provider to provide image URL/bytes #67693

merged 18 commits into from
May 30, 2023

Conversation

santihernandezc
Copy link
Contributor

@santihernandezc santihernandezc commented May 3, 2023 •
edited
Loading

Description

This PR implements the images.Provider interface described in our grafana/alerting repository (link).

This new implementation has two methods:

  • GetImageURL(): Gets a URL that can be used directly in our notifications.
  • GetRawImage(): Returns an io.ReadCloser to read the bytes of an image, useful for integrations that require the images to be uploaded.

Other Changes

  • ImageStore -> Provider (naming change).
  • Alerting repository updated in go.mod.
  • New URLExists() method in our store.

@santihernandezc santihernandezc changed the title (WIP) Refactor the ImageStore interface to work with our latest alert… Alerting: Refactor the ImageStore interface to provide image URL/bytes May 5, 2023
santihernandezc
santihernandezc commented May 5, 2023
View reviewed changes
pkg/services/ngalert/notifier/email_test.go Outdated
@@ -200,7 +200,7 @@ func createSut(t *testing.T, messageTmpl string, subjectTmpl string, emailTmpl *
},
Message: messageTmpl,
Subject: subjectTmpl,
}, receivers.Metadata{}, emailTmpl, ns, &images.UnavailableImageStore{}, &alertingLogging.FakeLogger{})
}, receivers.Metadata{}, emailTmpl, ns, &images.UnavailableProvider{}, &alertingLogging.FakeLogger{})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our interface was previously called images.ImageStore. but this name has recently been changed to images.Provider in this PR to better reflect its behavior.

All changes from ImageStore -> Provider respond to this name change in our alerting repository.

@santihernandezc santihernandezc added this to the 10.0.0 milestone May 5, 2023
@santihernandezc santihernandezc marked this pull request as ready for review May 6, 2023 00:28
@santihernandezc santihernandezc requested a review from a team May 6, 2023 00:28
@santihernandezc santihernandezc requested a review from a team as a code owner May 6, 2023 00:28
@santihernandezc santihernandezc requested review from sakjur, papagian and zserge and removed request for a team, sakjur, papagian and zserge May 6, 2023 00:28
yuri-tceretian
yuri-tceretian reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. One thing I would like us to have is logging that would mention how annotation was resolved, whether there was a record in db and if it was expired or not.

@santihernandezc santihernandezc changed the title Alerting: Refactor the ImageStore interface to provide image URL/bytes Alerting: Refactor the ImageStore/Provider to provide image URL/bytes May 10, 2023
@gotjosh gotjosh requested a review from JohnnyQQQQ May 15, 2023 15:11
JohnnyQQQQ
JohnnyQQQQ requested changes May 16, 2023
View reviewed changes
Copy link
Member

@JohnnyQQQQ JohnnyQQQQ left a comment

Choose a reason for hiding this comment

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

I did a first pass, looking good. I got one nit and one thing we should consider regarding the database fields we use in the where clause of the newly introduced URLExists function.

JohnnyQQQQ
JohnnyQQQQ approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@JohnnyQQQQ JohnnyQQQQ left a comment

Choose a reason for hiding this comment

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

LGTM, nice work 👍

@santihernandezc santihernandezc requested a review from a team as a code owner May 30, 2023 13:52
@santihernandezc santihernandezc merged commit 72a187b into main May 30, 2023
@santihernandezc santihernandezc deleted the santihernandezc/refactor_image_store_interface branch May 30, 2023 14:25
@alexweav alexweav mentioned this pull request May 30, 2023
Merged
alexweav added a commit that referenced this pull request May 30, 2023
@alexweav
Revert "Alerting: Refactor the ImageStore/Provider to provide image U…
a3a0c76 
…RL/bytes (#67693)"

This reverts commit 72a187b.
alexweav added a commit that referenced this pull request May 30, 2023
@alexweav
Revert "Alerting: Refactor the ImageStore/Provider to provide image U…
0ed5d3b 
…RL/bytes" (#69265)

Revert "Alerting: Refactor the ImageStore/Provider to provide image URL/bytes (#67693)"

This reverts commit 72a187b.
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview, 10.1.x May 31, 2023
@santihernandezc santihernandezc mentioned this pull request Jun 15, 2023
Merged
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 24, 2023
Merged
@JacobsonMT JacobsonMT mentioned this pull request Feb 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnnyQQQQ JohnnyQQQQ JohnnyQQQQ approved these changes

@yuri-tceretian yuri-tceretian yuri-tceretian approved these changes

@alexweav alexweav Awaiting requested review from alexweav

Assignees
No one assigned
Labels
add to changelog area/alerting Grafana Alerting area/backend/db/migration area/backend kata:am_unify no-backport Skip backport of PR
Projects
Alerting
Archived in project
Milestone
10.1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@santihernandezc @JohnnyQQQQ @yuri-tceretian @zerok @ricky-undeadcoders @grafanabot