Skip to content

Conversation

@ferruzzi
Copy link
Contributor

Follows the template set forward in #24643

Fixed, squashed, and rebased version of #26683 which I can't reopen.


# This test needs watcher in order to properly mark success/failure
# when "tearDown" task with trigger rule is part of the DAG
list(dag.tasks) >> watcher()
Copy link
Member

Choose a reason for hiding this comment

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

This additional list calls is unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for system tests which have trigger_rule='ALL_DONE' on one of the tasks. Even if either of the first two tasks fail, the third (delete_db_instance) will trigger, and the test will still mark as a pass without the watcher.

Copy link
Member

Choose a reason for hiding this comment

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

Weird, I thought dag.tasks already returns a (new) list

airflow/airflow/models/dag.py

Lines 1163 to 1165 in 685f523

@property
def tasks(self) -> list[Operator]:
return list(self.task_dict.values())

Copy link
Contributor Author

@ferruzzi ferruzzi Sep 28, 2022

Choose a reason for hiding this comment

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

My apologies, I misunderstood and thought you were saying the whole line was not needed.

Yeah, interesting. That block is a copy/paste from the AIP-47 specification, and all of the previous system tests do it this way. But it does look like maybe it isn't required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to keep it standardized like all of the others and fix them all later, unless you feel strongly about changing this one.

Copy link
Member

Choose a reason for hiding this comment

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

There was a good reason we added it first and possibly it has changed since by modifying "tasks" property to return list. As @ferruzzi wrote - better to keep it consistent and we might want to remove the list as a final refactor.

@potiuk potiuk merged commit db02c29 into apache:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants