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

add private method to reinitialize alerts #76

Merged
merged 6 commits into from
Jan 26, 2022
Merged

add private method to reinitialize alerts #76

merged 6 commits into from
Jan 26, 2022

Conversation

sed-i
Copy link
Contributor

@sed-i sed-i commented Jan 24, 2022 •
edited
Loading

This PR adds a private method for refreshing (re-initializing) relation data for alerts.
This is needed for cos-config to be able to refresh alerts when alerts show up on disk after the last hook fired.

cf. canonical/prometheus-k8s-operator#208, canonical/grafana-k8s-operator#53

This was referenced Jan 24, 2022
Merged
Merged
dstathis
dstathis reviewed Jan 24, 2022
View reviewed changes
Copy link
Contributor

@dstathis dstathis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This seems to adding a new method and calling it nowhere. Could you explain?

I should have read your first comment.

dstathis
dstathis previously approved these changes Jan 24, 2022
View reviewed changes
@sed-i sed-i mentioned this pull request Jan 25, 2022
Merged
10 tasks
mmanciop
mmanciop previously requested changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@mmanciop mmanciop left a comment

Choose a reason for hiding this comment

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

Code looks reasonable, please add a few unit tests to make sure it works as we expect when we do add, change or remove alert rules.

mmanciop
mmanciop approved these changes Jan 25, 2022
View reviewed changes
Copy link
Contributor

@mmanciop mmanciop left a comment

Choose a reason for hiding this comment

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

PR description needs update (method no longer public), the rest LGTM

@sed-i sed-i changed the title add refresh method add private method to reinitialize alerts Jan 25, 2022
@sed-i sed-i mentioned this pull request Jan 26, 2022
Merged
@simskij simskij merged commit 7d704b1 into main Jan 26, 2022
@simskij simskij deleted the feature/extra_listen_event branch January 26, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbarry82 rbarry82 rbarry82 approved these changes

@mmanciop mmanciop mmanciop approved these changes

@dstathis dstathis dstathis approved these changes

@simskij simskij Awaiting requested review from simskij

@balbirthomas balbirthomas Awaiting requested review from balbirthomas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sed-i @dstathis @mmanciop @rbarry82 @simskij