Skip to content

[AIRFLOW-1571] Add AWS Lambda Hook#2570

Closed
sid88in wants to merge 5 commits into
apache:masterfrom
sid88in:feature/aws_lambda_hook
Closed

[AIRFLOW-1571] Add AWS Lambda Hook#2570
sid88in wants to merge 5 commits into
apache:masterfrom
sid88in:feature/aws_lambda_hook

Conversation

@sid88in

@sid88in sid88in commented Sep 6, 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 closed this Sep 6, 2017
@sid88in sid88in reopened this Sep 6, 2017
@codecov-io

codecov-io commented Sep 6, 2017

Copy link
Copy Markdown

Codecov Report

Merging #2570 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
+ Coverage   71.13%   71.14%   +<.01%     
==========================================
  Files         150      150              
  Lines       11741    11741              
==========================================
+ Hits         8352     8353       +1     
+ Misses       3389     3388       -1
Impacted Files Coverage Δ
airflow/models.py 87.15% <0%> (+0.04%) ⬆️

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 a81c153...6a96caf. Read the comment docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we textwrap.dedent here instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks @saguziel for reviewing my PR :) Pushed the change.

@sid88in sid88in closed this Sep 7, 2017
@sid88in sid88in reopened this Sep 7, 2017
@sid88in

sid88in commented Sep 7, 2017

Copy link
Copy Markdown
Contributor Author

@artwr @criccomini (+ @saguziel @bolkedebruin) seems like you guys reviewed similar AWS related PR - #1630

Would be great if you could help review my PR. Any inputs are welcome (this is my first contribution to airflow) :)

@bolkedebruin

Copy link
Copy Markdown
Contributor

Please make sure your commits are merged into one.

@sid88in

sid88in commented Sep 8, 2017

Copy link
Copy Markdown
Contributor Author

@bolkedebruin resolved commits. thanks

@sid88in
sid88in force-pushed the feature/aws_lambda_hook branch from 71fb066 to 91c6845 Compare September 9, 2017 22:23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You need some :paramname: and :type: documentation for the Class docstring, for the constructor properties
e.g.
:param function_name: The name of the AWS Lambda function to call
:type function_name: string

etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch Thanks. ! I'll add docstrings today to both #2570 and #2586

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looking through the contrib codebase I see a lot of missing docstrings and param info, which is a shame because it's then missing when generating the docs, so maybe I will contribute to improving that.
But generally I can't see any other issues with the code, so it's a great effort! At some point soon I will be using your dynamodb stuff heavily, so if it's not in a release by then I will borrow it for my plugins folder 😀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks @davoscollective for all the positive energy :)

@sid88in
sid88in force-pushed the feature/aws_lambda_hook branch from e2bbbba to 6a96caf Compare September 20, 2017 22:11
@sid88in
sid88in force-pushed the feature/aws_lambda_hook branch 4 times, most recently from fb7fb12 to c542b9c Compare October 12, 2017 03:44
@sid88in
sid88in force-pushed the feature/aws_lambda_hook branch from 8b5de04 to d448b18 Compare October 13, 2017 20:13
@sid88in

sid88in commented Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Closing this PR - its a duplicate of #2718

@sid88in sid88in closed this Oct 23, 2017
@sid88in
sid88in deleted the feature/aws_lambda_hook branch October 23, 2017 05:23
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