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

Added Telegram Notification Lambda function #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abutaha
Copy link

@abutaha abutaha commented Feb 1, 2017

Added Telegram Notification Lambda function

Copy link
Contributor

@jaymccon jaymccon left a comment

Choose a reason for hiding this comment

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

overall looks pretty good, aside from the specific comments, I'd suggest passing your code through flake8 to cleanup the few (often pedantic) style issues it reports

telegram-notifier/LambdaFunction.py Show resolved Hide resolved

logger.info(str(msg))

req = Request(API_ENDPOINT, urllib.urlencode(msg))
Copy link
Contributor

Choose a reason for hiding this comment

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

using the requests module here will probably be more concise, and probably more familiar to python coders.

as an added plus there's no need to bundle the lib in your deployment package as it's included in botocore:

import botocore.vendored.requests as requests

@@ -0,0 +1,40 @@
from __future__ import print_function

import boto3
Copy link
Contributor

Choose a reason for hiding this comment

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

boto3, json and os modules are being imported without being needed

tipuq pushed a commit that referenced this pull request Feb 6, 2017
File references something which is not present
@robperc robperc mentioned this pull request Jun 26, 2017
tipuq pushed a commit that referenced this pull request Oct 6, 2023
Update risk_credentials_exposed.serverless.yaml with all possible eve…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants