fix: TriggerDagRunOperator deferral mode not working for Airflow 3

[kacpermuda]

#57369 broke the value returned by DagStateTrigger, instead of a tuple it started to return a dict, while the operator still expects a tuple. As for some time we've been doing a tuple, I reverted that change.

This is the failing line, after coming back from execute.

I tried to make it as clear as possible that it should be a tuple and what to expect from its content.

---

^ 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 airflow-core/newsfragments.

[kacpermuda]

@potiuk, the PR that broke it was not included in changelog as it was titled as mypy fixes. Not blaming here or anything, just letting you know - not sure how we can improve the process for RM so it's easier to decide what should and should not be included in Changelog.

[kacpermuda]

@uranusjr - addressed the comments, it's ready for re-review, thanks !

[potiuk]

@potiuk, the PR that broke it was not included in changelog as it was titled as mypy fixes. Not blaming here or anything, just letting you know - not sure how we can improve the process for RM so it's easier to decide what should and should not be included in Changelog.

Whether something is in a changelog or not, does not matter when it's breaking something.

So It does not matter for RM to be honest. RM (and you can look it up in https://infra.apache.org/release-publishing.html#releasemanager - release manager's work is purely mechanical. Release Manager manages the mechanics of the release, not whether particular PR is breaking some subtle aspects of the product. It's more of the initial contributor's role to make sure the regression tests are good enough, and reviewer's case to verify. RM has to trust that if the PR is green and been reviewed, that it does what is described in the PR description.

This failure here is not really a failure of a release process but either:

• tests were not comprhensive enough to catch a regression
  or
• reviews did not catch it

Once those two are passed, RM might make a third pass (and sometimes does) - but there is no physical possibility (nor is not RM responsibility) to be able to check if a change has side effect.

I guess in this case it's the matter of tests that should be written in the way that they fail if things change and it should be clear that the tests are not "accidental" and that they expect tuple to be returned.

Or am I wrong?

How else would you want to expect that RM should know that things are broken if the tests pass and reviewer said "ok" and someone merged it?

[potiuk]

And - just to be clear - I am not trying to "deny" any kind of responsibility, but simply stating the fact that "breaking/not breaking" is not the matter of changelog. If we add this entry to the changelog, it would not have it even a BIT clearer that it was the cause of the problem. This is also why in providers we have TWO sources of information:

Changelog that shows RM-decided "imporant changes"

• https://airflow.apache.org/docs/apache-airflow-providers-standard/stable/changelog.html

Detailed list of commits:

• https://airflow.apache.org/docs/apache-airflow-providers-standard/stable/commits.html

Which lists ALL comits - no matter what the description and assesment of the RM based on the description and brief overview of the change by looking at the PR was.

The second list is what those who are really diving deep should look like. And we deliberately made it auto-generated as part of the release so that when you want to dive deeper without just running git log on the folder.

I am not sure if that part of the process can be included, but I am all ears if there is any idea.

[kacpermuda]

Yes, that makes sense. Even after writing this first comment I realized it's not RM responsibility. I believe the title was a bit confusing there and suggested only typing is changed + as you said, the tests were not comprehensive enough. Sorry for the confusion. Also thanks for reminding about list of commits being in the docs, I usually used git diff in IDE, but maybe this will be easier.

[potiuk]

Yes, that makes sense. Even after writing this first comment I realized it's not RM responsibility. I believe the title was a bit confusing there and suggested only typing is changed + as you said, the tests were not comprehensive enough. Sorry for the confusion. Also thanks for reminding about list of commits being in the docs, I usually used git diff in IDE, but maybe this will be easier.

You can still use git diff - I do all the time - :) - it's really for the advantage of the users who might want to dig in but also those who are testing the provider.

Also - this list of commits includes ALL committs even those where providers were moved in the repository - which is not at all given with git diff - git can easily track single file moves/renames, but good luck if you want to track directory moves - the list of commits we generate has the logic to also show changes for such moved providers

[kacpermuda]

Test failure seems unrelated, some GH failure

[kacpermuda]

All looks green, Would appreciate a merge and backport to AF 3.1

[jason810496]

Would appreciate a merge and backport to AF 3.1

However, I found that the scope of the current PR belongs to standard provider.
So it seems we don't need to backport as it's proivider instead of core.

[kacpermuda]

Ah, sorry, correct, I mixed up the PRs. This is the one I wanted to backport.