Skip to content

Fix region_name being ignored by the Step Functions execution trigger - #72625

Merged
vincbeck merged 2 commits into
apache:mainfrom
nandeshkanagaraju:add-step-function-trigger-tests
Sep 9, 2026
Merged

vincbeck merged 2 commits into
apache:mainfrom
nandeshkanagaraju:add-step-function-trigger-tests

Conversation

@nandeshkanagaraju

@nandeshkanagaraju nandeshkanagaraju commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

StepFunctionsExecutionCompleteTrigger accepted region_name but only put it into serialized_fields without forwarding it to AwsBaseWaiterTrigger, so self.region_name stayed None and the deferred waiter always polled Step Functions in the default region instead of the one the operator was configured with. The base class already serializes region_name itself, so the serialized_fields entry was redundant on top of being inert.

Also adds the previously missing dedicated test module for the trigger (tracked in the OVERLOOKED_TESTS allowlist). test_hook_forwards_connection_config is the regression test for this fix — it is the one that fails without the production change; test_serialization and test_run_success are there for module coverage and pass either way.

Claude-Session: https://claude.ai/code/session_01QL3YuSRJuK6WpZGc4jMjd1


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Opus 5)

Generated-by: Claude Code (Opus 5) following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.

@boring-cyborg boring-cyborg Bot added area:providers provider:amazon AWS/Amazon - related issues labels Sep 7, 2026
@boring-cyborg

boring-cyborg Bot commented Sep 7, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@rjgoyln rjgoyln left a comment

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.

LGTM overall! Verified the fix locally and confirmed it is correct and minimal. The relevant tests and ruff checks are clean. Just my thoughts — two small nits below. Feel free to resolve if you think otherwise.

Comment thread providers/amazon/tests/unit/amazon/aws/triggers/test_step_function.py Outdated

@rjgoyln rjgoyln left a comment

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.

Nice 🙂!

StepFunctionsExecutionCompleteTrigger accepted region_name but only put
it into serialized_fields without forwarding it to AwsBaseWaiterTrigger,
so self.region_name stayed None and the deferred waiter always polled
Step Functions in the default region instead of the one the operator was
configured with. The base class already serializes region_name itself,
so the serialized_fields entry was redundant on top of being inert.

Also adds the previously missing dedicated test module for the trigger
(tracked in the OVERLOOKED_TESTS allowlist), including a regression test
that fails without this fix.

Claude-Session: https://claude.ai/code/session_01QL3YuSRJuK6WpZGc4jMjd1
The patched get_async_conn is the method, so the async context manager
lives on its return value; setting __aenter__ on the mock itself was a
no-op that passed only because AsyncMock auto-creates the manager.
@nandeshkanagaraju
nandeshkanagaraju force-pushed the add-step-function-trigger-tests branch from acc5052 to ea19177 Compare September 8, 2026 18:04
@vincbeck
vincbeck merged commit ff95763 into apache:main Sep 9, 2026
83 checks passed
@boring-cyborg

boring-cyborg Bot commented Sep 9, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

bingqin2 added a commit to bingqin2/airflow that referenced this pull request Sep 9, 2026
…mpleteTrigger

The region_name fix originally proposed in this PR landed via apache#72625. This adds the remaining coverage: the error TriggerEvent path, verify/botocore_config serialization, pruning of unset hook parameters, and the region_name docstring.
vincbeck pushed a commit that referenced this pull request Sep 9, 2026
…mpleteTrigger (#72570)

The region_name fix originally proposed in this PR landed via #72625. This adds the remaining coverage: the error TriggerEvent path, verify/botocore_config serialization, pruning of unset hook parameters, and the region_name docstring.
imrichardwu pushed a commit to imrichardwu/airflow that referenced this pull request Sep 11, 2026
…apache#72625)

StepFunctionsExecutionCompleteTrigger accepted region_name but only put
it into serialized_fields without forwarding it to AwsBaseWaiterTrigger,
so self.region_name stayed None and the deferred waiter always polled
Step Functions in the default region instead of the one the operator was
configured with. The base class already serializes region_name itself,
so the serialized_fields entry was redundant on top of being inert.

Also adds the previously missing dedicated test module for the trigger
(tracked in the OVERLOOKED_TESTS allowlist), including a regression test
that fails without this fix.
imrichardwu pushed a commit to imrichardwu/airflow that referenced this pull request Sep 11, 2026
…mpleteTrigger (apache#72570)

The region_name fix originally proposed in this PR landed via apache#72625. This adds the remaining coverage: the error TriggerEvent path, verify/botocore_config serialization, pruning of unset hook parameters, and the region_name docstring.
xvega pushed a commit to xvega/airflow that referenced this pull request Sep 13, 2026
…mpleteTrigger (apache#72570)

The region_name fix originally proposed in this PR landed via apache#72625. This adds the remaining coverage: the error TriggerEvent path, verify/botocore_config serialization, pruning of unset hook parameters, and the region_name docstring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants