-
Notifications
You must be signed in to change notification settings - Fork 10
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
Introduce a new JSON notifier #138
Conversation
a1ab683
to
f9e85a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More changes are needed in the documentation. In the README, we state
Right now we only support Discord and Slack and Telegram, but if you need a specific notifier, feel free to open an issue.
Please update this. Maybe since we're starting to support quite a few, we can just remove this altogether.
blackbox/handlers/notifiers/json.py
Outdated
payload = [] | ||
for database in self.report.databases: | ||
database_payload = { | ||
"source": database.database_id, | ||
"success": database.success, | ||
"output": database.output or None | ||
} | ||
storages_payload = [] | ||
for provider in database.storages: | ||
storages_payload.append({"name": provider.storage_id, "success": provider.success}) | ||
database_payload['backup'] = storages_payload | ||
payload.append(database_payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly cramped with no newlines here, and could use a comment or two so it's easier to skim. Take a peek at the Discord notifier for an example of how this could be made a little bit more readable.
We're trying to keep this codebase extremely approachable and beginner-friendly.
Code looks good, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that, updated in eea20a1
|
||
return {"backup-data": payload} | ||
|
||
def notify(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a docstring here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in eea20a1
pyproject.toml
Outdated
@@ -3,7 +3,7 @@ name = "blackbox-cli" | |||
packages = [ | |||
{ include = "blackbox" } | |||
] | |||
version = "v2.3.2" | |||
version = "v2.4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't increment the version number manually, this is handled automatically by CI/CD, see https://github.com/lemonsaurus/blackbox/blob/main/.github/workflows/publish.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased all the commits interactively to keep the history clean, so this bump is no longer part of d434891
This will allow us to send notifications independently of any particular app (Discord, slack, etc.)
f9e85a5
to
d434891
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good now, sorry it's taken so long to review.
Closes #108
This pull request introduces a plain
JSON
notifier that'll allow us to decoupleblackbox
from any particular application (Discord
,Slack
orTelegram
).The request's body can still be changed to whatever we will agree upon in later discussions.