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.

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

Merged
merged 10 commits into from
Apr 24, 2024
Merged

feat: Improved AWS backup notification readability #222

merged 10 commits into from
Apr 24, 2024

Conversation

falense
Copy link
Contributor

@falense falense commented Apr 3, 2024

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

Screenshot from 2024-04-03 08-08-24

After

Screenshot from 2024-04-03 08-07-47

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.

Copy link
Member

@antonbabenko antonbabenko left a 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?

@falense
Copy link
Contributor Author

falense commented Apr 5, 2024

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.

@falense
Copy link
Contributor Author

falense commented Apr 16, 2024

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?

./notify_slack.py:290:35: E711 comparison to None should be 'if cond is None:'
./notify_slack.py:298:38: E711 comparison to None should be 'if cond is None:'
./notify_slack.py:307:52: E203 whitespace before ':'

@bryantbiggs
Copy link
Member

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

@falense
Copy link
Contributor Author

falense commented Apr 24, 2024

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

image

I have done another iteration on the tests. Please approve the workflows.

@falense
Copy link
Contributor Author

falense commented Apr 24, 2024

I have added you both as maintainers on the fork. Maybe that will do the trick?

@antonbabenko
Copy link
Member

Maybe, I will take a look in a few hours today.

@falense
Copy link
Contributor Author

falense commented Apr 24, 2024

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 pipenv run format followed by pipenv run lint

@antonbabenko
Copy link
Member

I looked at it, but you still need to fix the failing unit tests and formatting to make GitHub Actions green.

Copy link
Member

@bryantbiggs bryantbiggs left a 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!

cc @antonbabenko

@bryantbiggs bryantbiggs merged commit 27d1c46 into terraform-aws-modules:master Apr 24, 2024
8 checks passed
antonbabenko pushed a commit that referenced this pull request Apr 24, 2024
## [6.4.0](v6.3.0...v6.4.0) (2024-04-24)

### Features

* Improved AWS backup notification readability ([#222](#222)) ([27d1c46](27d1c46))
@antonbabenko
Copy link
Member

This PR is included in version 6.4.0 🎉

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants