-
-
Notifications
You must be signed in to change notification settings - Fork 343
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
feat: Improved AWS backup notification readability #222
feat: Improved AWS backup notification readability #222
Conversation
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.
Looks good. Could you please update tests like described here also?
I have added a test according to the examples. Did I do it right? I had some troubles with the somewhat older Python3.8 version so I bumped it to 3.10 to be able to generate the snapshot. |
I saw that there were more tests. I have corrected the style errors listed below. Is there anything else I can do to get this merged?
|
you'll need to get the CI checks to pass - unfortunately, you have disabled our ability to update your branch so you will have to do these updates on your end |
Thank you for your reply! Enabling maintainer edits checkbox is missing on this PR. Not sure if this is a Github issue or if it has to be enabled when creating the PR. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork I have done another iteration on the tests. Please approve the workflows. |
I have added you both as maintainers on the fork. Maybe that will do the trick? |
Maybe, I will take a look in a few hours today. |
Brainstormed this with some colleagues. Black suggests adding ignoring E203 and E701 as a minimal config. See https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#minimal-configuration. I have added the change to the PR. Format adds whitespaces and linting fails because of it. You can try this by running |
I looked at it, but you still need to fix the failing unit tests and formatting to make GitHub Actions green. |
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.
Added in all of the notification examples from here https://docs.aws.amazon.com/aws-backup/latest/devguide/backup-notifications.html to capture in the snapshots. Not sure why the snapshottest library decided to reformat everything - thats rather unfortunate and noisy but all looks good!
## [6.4.0](v6.3.0...v6.4.0) (2024-04-24) ### Features * Improved AWS backup notification readability ([#222](#222)) ([27d1c46](27d1c46))
This PR is included in version 6.4.0 🎉 |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
When using this module with AWS backup events the resulting message is poorly formatted due to the unstructured nature of the message sent by AWS backup. This PR does a best effort parsing of the message in order to improve readability for the user.
Motivation and Context
Before
After
Breaking Changes
This should not cause any breaking changes and only affects AWS backup messages
How Has This Been Tested?
We have been running on our test environment for a couple of weeks now.