Skip to content

Comments

Replace external_trigger check with DagRunType#45961

Merged
Lee-W merged 5 commits intoapache:mainfrom
jason810496:replace-external_trigger-with-dag-run-type
Feb 25, 2025
Merged

Replace external_trigger check with DagRunType#45961
Lee-W merged 5 commits intoapache:mainfrom
jason810496:replace-external_trigger-with-dag-run-type

Conversation

@jason810496
Copy link
Member

closes: #45932


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:db-migrations PRs with DB migration area:dev-tools area:providers area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation provider:openlineage AIP-53 labels Jan 23, 2025
@Lee-W Lee-W added the legacy api Whether legacy API changes should be allowed in PR label Jan 23, 2025
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch 3 times, most recently from 45cf835 to 9eef4f1 Compare January 24, 2025 06:48
@jason810496 jason810496 changed the title [WIP] Replace external_trigger check with DagRunType Replace external_trigger check with DagRunType Jan 24, 2025
@jason810496 jason810496 marked this pull request as ready for review January 24, 2025 08:56
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This also needs a newsfragment noting about the removal of it please

@ashb ashb added the airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes label Jan 24, 2025
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch 2 times, most recently from 2fee34c to a385e61 Compare January 28, 2025 17:44
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch from d69f748 to 3d7605b Compare February 21, 2025 01:44
@Lee-W Lee-W self-requested a review February 21, 2025 03:51
@mobuchowski mobuchowski removed their request for review February 21, 2025 08:22
@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch from 3d7605b to 5ff5563 Compare February 22, 2025 11:00
@jason810496
Copy link
Member Author

Only non-related ts-compile-format-lint-ui static check fail 🎉

@jason810496 jason810496 force-pushed the replace-external_trigger-with-dag-run-type branch from 5ff5563 to cbe167a Compare February 24, 2025 12:42
@uranusjr
Copy link
Member

Not unrelated, those lines reference the removed external_trigger field.

@uranusjr
Copy link
Member

I added a commit that should fix the static checks.

jason810496 and others added 4 commits February 25, 2025 16:34
- Fix test_handle_multiple_columns_unique_constraint_error

- Resolve ashb code review

- Add newsfragment

- Replace external_trigger is False logic with run_type is SCHEDULED

- Fix _emit_true_scheduling_delay_stats_for_finished_state

- Fix migrations fix after rebasing to latest main

Fix test_exceptions and static check after rebasing

Fix test_dag_run and erd static check
The 'triggered_by' field used to only be shown when the run is
externally triggered. Rather than using run_type to maintain the
conditional, I decided to render the field unconditionally instead,
since the value could be useful for scheduler-created runs too (for
example, distinguishing whether a run is asset-triggered or
time-scheduled).
@Lee-W Lee-W force-pushed the replace-external_trigger-with-dag-run-type branch from fcf4e97 to aba5eec Compare February 25, 2025 08:49
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

LGTM, one small change needed in task_sdk as a result though please.

@Lee-W Lee-W merged commit 811fa2b into apache:main Feb 25, 2025
90 checks passed
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 28, 2025
* Replace external_trigger check with DagRunType

- Fix test_handle_multiple_columns_unique_constraint_error

- Resolve ashb code review

- Add newsfragment

- Replace external_trigger is False logic with run_type is SCHEDULED

- Fix _emit_true_scheduling_delay_stats_for_finished_state

- Fix migrations fix after rebasing to latest main

Fix test_exceptions and static check after rebasing

Fix test_dag_run and erd static check

* fixup! Fix run_type logic, remove unused externally_triggered_type

* fixup! Restore external_trigger value when downgrade

* Remove externally_trigger reference in frontend

The 'triggered_by' field used to only be shown when the run is
externally triggered. Rather than using run_type to maintain the
conditional, I decided to render the field unconditionally instead,
since the value could be useful for scheduler-created runs too (for
example, distinguishing whether a run is asset-triggered or
time-scheduled).

* docs(newsfragments): add migration rules

---------

Co-authored-by: Tzu-ping Chung <[email protected]>
Co-authored-by: Wei Lee <[email protected]>
nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025
* Replace external_trigger check with DagRunType

- Fix test_handle_multiple_columns_unique_constraint_error

- Resolve ashb code review

- Add newsfragment

- Replace external_trigger is False logic with run_type is SCHEDULED

- Fix _emit_true_scheduling_delay_stats_for_finished_state

- Fix migrations fix after rebasing to latest main

Fix test_exceptions and static check after rebasing

Fix test_dag_run and erd static check

* fixup! Fix run_type logic, remove unused externally_triggered_type

* fixup! Restore external_trigger value when downgrade

* Remove externally_trigger reference in frontend

The 'triggered_by' field used to only be shown when the run is
externally triggered. Rather than using run_type to maintain the
conditional, I decided to render the field unconditionally instead,
since the value could be useful for scheduler-created runs too (for
example, distinguishing whether a run is asset-triggered or
time-scheduled).

* docs(newsfragments): add migration rules

---------

Co-authored-by: Tzu-ping Chung <[email protected]>
Co-authored-by: Wei Lee <[email protected]>
ntBre pushed a commit to astral-sh/ruff that referenced this pull request Feb 9, 2026
…context key for Airflow 3.0 (`AIR301`) (#22850)

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->
Context:
apache/airflow#41641

1. apache/airflow#45961
* <strike>create_dagrun removed from airflow...DAG</strike> (This has
already been implemented in AIR301)
* context key dag_run.external_trigger removed
2. apache/airflow#45960
* context["inlet_events"]["url"] → context["inlet_events"][Asset("url")]
3. apache/airflow#41348
* context key triggering_dataset_events → triggering_asset_events

The existing AIR301 rules can detect when users access a removed key
such as `execution_date` through Airflow's `context`. For example,
`context["execution_date"]`, or `context["dag_run"]`. However, if an
attribute is deprecated from a context key, such as
`context["dag_run"].external_trigger`, the current implement will not
flag it.

This PR adds the logic for such check, and add two rules to flag the
deprecated attribute for the `"dag_run"` and `"inlet_events"` context
key. In addition to this, `"triggering_dataset_events"` is a deprecated
context key which can be handled by the existing rule. However, the
existing rule doesn't raise a diagnostic. Hence, the rule logic is
refactored a little bit, such that we can add this check and suggest a
`Replacement::Rename`.

## Test Plan

<!-- How was it tested? -->
The test cases have been added to `AIR301_context.py`, and all the tests
have been run locally and success. @Lee-W , could you please review it
when you have time, thanks!

## Notes

In #22376, we introduced some
improvements to the AIR301 code. I will re-base this PR when we are all
good on that, so it can pick up those code structure improvements, and
updated rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes area:API Airflow's REST/HTTP API area:CLI area:db-migrations PRs with DB migration area:dev-tools area:providers area:Scheduler including HA (high availability) scheduler area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation legacy api Whether legacy API changes should be allowed in PR provider:openlineage AIP-53

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace external_trigger check with DagRunType

5 participants