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

Migrate notifications to the go-pkgz/notify module #1338

Merged
merged 5 commits into from
Apr 29, 2022
Merged

Migrate notifications to the go-pkgz/notify module #1338

merged 5 commits into from
Apr 29, 2022

Conversation

paskal
Copy link
Collaborator

@paskal paskal commented Apr 26, 2022 •
edited
Loading

Notifications still work, I haven't tested webhook and slack but tested email and telegram:
image
image

@paskal paskal added the backend label Apr 26, 2022
@paskal paskal requested a review from umputun as a code owner April 26, 2022 00:31
@paskal paskal marked this pull request as draft April 26, 2022 07:29
@coveralls
Copy link

coveralls commented Apr 26, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2245401540

  • 118 of 125 (94.4%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 83.945%
Changes Missing Coverage Covered Lines Changed/Added Lines %
backend/app/notify/telegram.go 32 33 96.97%
backend/app/cmd/server.go 10 16 62.5%
Files with Coverage Reduction New Missed Lines %
backend/app/cmd/server.go 1 82.33%
Totals Coverage Status
Change from base Build 2224620814: -0.07%
Covered Lines: 5694
Relevant Lines: 6783
💛 - Coveralls

@paskal paskal marked this pull request as ready for review April 26, 2022 07:44
@paskal paskal force-pushed the paskal/notify_migration branch 2 times, most recently from f8f5139 to cf5f6d6 Compare April 28, 2022 00:33
@paskal paskal changed the title Notify telegram notifications to the go-pkgz/notify module Move telegram, slack, webhook notifications to the go-pkgz/notify module Apr 28, 2022
@paskal paskal changed the title Move telegram, slack, webhook notifications to the go-pkgz/notify module Migrate notifications to the go-pkgz/notify module Apr 29, 2022
@paskal
Copy link
Collaborator Author

paskal commented Apr 29, 2022

Done for all modules, I separated commits by modules for easier review.

umputun
umputun reviewed Apr 29, 2022
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM. However, the part " I haven't tested webhook and slack" makes me a little bit uncomfortable. Could you please play with webhooks and slack a little bit, just in case?

@paskal
Copy link
Collaborator Author

paskal commented Apr 29, 2022

Webhook and Slack works as well:
image
image

@umputun umputun merged commit a980bdb into master Apr 29, 2022
@umputun umputun deleted the paskal/notify_migration branch April 29, 2022 18:32
@paskal paskal added this to the 1.10.0 milestone Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umputun umputun umputun left review comments

Assignees
No one assigned
Labels
backend
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
3 participants
@paskal @coveralls @umputun