-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Feature: Add invoke lambda function operator #21686
Feature: Add invoke lambda function operator #21686
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have raised a couple of code-level issues for your consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This needs a major version bump in the Amazon provider.
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
Looks like I missed adding documentation and an example DAG. I've updated the PR with the same.
A few QQs: What does it mean if a major version bump is required? Is there anything required on my part? What usually qualifies for a major version bump for provider packages? |
It means we need to be careful when including this change in a release on PyPI. Nothing you need to do here. |
Sorry, I submitted those as single comments. Wish I could retroactively roll them into a review to reduce clutter. |
It's no trouble at all, thank you for the review @ferruzzi. Allow me to work on these changes. It appears some tests are failing as well. Will work on the same. |
Tests and docs are failing :( |
Apologies for the trouble @potiuk. Working on the fixes today. Let me convert this into a draft. |
Fixes pushed! Hope I got it right this time. |
@malthe, @edwardwang888, @ferruzzi, @mik-laj, would y'all also be able to provide further feedback/approval on the PR please? I hope the changes have addressed all existing comments. |
update operator and underlying hook code to explicitly specify invoke and create function arguments
modify test cases to use static method for lambda invoke operator remove redundant try-except block
update AWS lambda hook functions to enforce keyword only args
Awesome work, congrats on your first merged pull request! |
related: #21009
This change is to implement AWS Lambda function invoke operator for the AWS provider package.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.