Skip to content

[AIRFLOW-1571] Add AWS Lambda Hook#2718

Closed
sid88in wants to merge 1 commit into
apache:masterfrom
sid88in:feature/aws_lambda_hook2
Closed

[AIRFLOW-1571] Add AWS Lambda Hook#2718
sid88in wants to merge 1 commit into
apache:masterfrom
sid88in:feature/aws_lambda_hook2

Conversation

@sid88in

@sid88in sid88in commented Oct 23, 2017

Copy link
Copy Markdown
Contributor

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, no UI changes

Details of this PR are in the Jira above.

Tests

  • My PR adds the following unit tests
  1. test_aws_lambda_hook
    This unit test uses moto to mock aws lambda. Firstly, we create a lambda function (assuming this function is already created by aws cloudformation template in production). Secondly, we trigger lambda function using invoke function in the hook. Lastly, we parse the response and assert statements.

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

@sid88in sid88in mentioned this pull request Oct 23, 2017
4 tasks
@codecov-io

codecov-io commented Oct 23, 2017

Copy link
Copy Markdown

Codecov Report

Merging #2718 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2718   +/-   ##
=======================================
  Coverage   72.38%   72.38%           
=======================================
  Files         154      154           
  Lines       11815    11815           
=======================================
  Hits         8552     8552           
  Misses       3263     3263

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ef2b6c...7feefcf. Read the comment docs.

@sid88in
sid88in force-pushed the feature/aws_lambda_hook2 branch from a407890 to 7feefcf Compare October 23, 2017 13:50
@andyxhadji

andyxhadji commented Oct 23, 2017

Copy link
Copy Markdown
Contributor

@ reviewer - As part of a larger design decision, how should we think about naming hooks extending the AwsHook base class?

e.g. here we name the file "AwsLambda", but elsewhere we call it by the service name without the "Aws" prefix - "S3Hook" instead of "AwsS3Hook". This is also relevant for the RedshiftHook PR (#2717): "AwsRedshiftHook" vs. "RedshiftHook".

Just want to make sure our naming is consistent!

@sid88in

sid88in commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

@andyxhadji I concur with that. For aws related hooks/operators we should have consistent naming convention. For instance, google hooks start with gcp_* filename and GoogleCloudHookname For the same reason for dynamodb I merged aws_dynamodb_hook and for Kinesis #2586 its aws_kinesis_hook filenames and AwsHookname.

@andyxhadji

Copy link
Copy Markdown
Contributor

Sounds good to me, I'll make a JIRA issue to address naming inconsistencies with current Aws hook extensions in order to follow that practice.

@sid88in

sid88in commented Nov 1, 2017

Copy link
Copy Markdown
Contributor Author

@bolkedebruin would be great if you could please help review this?

asfgit pushed a commit that referenced this pull request Nov 2, 2017
Closes #2718 from sid88in/feature/aws_lambda_hook2

(cherry picked from commit cc01b7d)
Signed-off-by: Bolke de Bruin <bolke@xs4all.nl>
@asfgit asfgit closed this in cc01b7d Nov 2, 2017
@bolkedebruin

Copy link
Copy Markdown
Contributor

Merged. Another idea is to merge all Aws hooks into one file, it is getting a bit messy now. Operators maybe as well.

@sid88in

sid88in commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

yay thanks @bolkedebruin !! Yes I agree that we should combine aws related hooks in 1 file. Will create a new PR to add aws related hooks (EMR, DynamoDB and Lambda) in aws_hook.py.. once thats done, I am going to decline my PR #2586 and submit a fresh PR with the hook. Once we start adding more operators we can take similar approach. thanks again

@andyxhadji

andyxhadji commented Nov 2, 2017 via email

Copy link
Copy Markdown
Contributor

@sid88in

sid88in commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

@andyxhadji you raise some good points. I think we should think about restructuring with @bolkedebruin .

Acehaidrey pushed a commit to Acehaidrey/incubator-airflow that referenced this pull request Jan 19, 2018
Closes apache#2718 from sid88in/feature/aws_lambda_hook2
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.

4 participants