-
-
Notifications
You must be signed in to change notification settings - Fork 332
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 format of Auto Scaling notifications #76
Conversation
c7261b8
to
426c62e
Compare
looks like I forked while you were doing a deploy - rebased off the latest |
Thanks, Andrew! You are right, you pushed in a little bit wrong moment. I am still working on some maintenance tasks in the |
I had missed one conflict in the |
This PR has been automatically marked as stale because it has been open 30 days |
This PR was automatically closed because of stale in 10 days |
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
Added formatting for Auto Scaling Notifications, also noticed that the formatting for
AlarmName
was not using thenotification
so I refactored to match the other formats.Motivation and Context
I'm piping notifications for many things into my lambda & want to have pretty messages for them.
You could probably close #3 with this PR and your README.md which asks for PRs - would also close #28 since the author seems to have orphaned the PR.
Breaking Changes
Note my addition of
import certifi
- this was to get local testing running - should be OK on Lambda IMOP but did not test there.I think the current version probably has a bug in the formatting of the processing of notifications including the string
AlarmName
which I believe I fixed in this PR.How Has This Been Tested?
By publishing messages to my slack as described in the README.md
See PR for my tests - should be basic improvement/refactor - no big changes other than the inclusion of
certifi
- might want to think about that change, but local execution of the test was not possible without it (note that this change is in thenotify_slack.py
file, so would be part of the running production code).