ECSOperator / pass context to self.xcom_pull as it was missing (when using reattach)

[pmalafosse]

FYI @darwinyip

This is to fix a bug I found with context not being passed to xcom_pull (used for the reattach feature)

Traceback (most recent call last):
  File "/home/airflow/.local/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/__main__.py", line 40, in main
    args.func(args)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/cli_parser.py", line 48, in command
    return func(*args, **kwargs)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/cli.py", line 91, in wrapper
    return f(*args, **kwargs)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/commands/task_command.py", line 393, in task_test
    ti.run(ignore_task_deps=True, ignore_ti_state=True, test_mode=True)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 70, in wrapper
    return func(*args, session=session, **kwargs)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1432, in run
    self._run_raw_task(
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 67, in wrapper
    return func(*args, **kwargs)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1157, in _run_raw_task
    self._prepare_and_execute_task_with_callbacks(context, task)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1331, in _prepare_and_execute_task_with_callbacks
    result = self._execute_task(context, task_copy)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/models/taskinstance.py", line 1356, in _execute_task
    result = task_copy.execute(context=context)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/utils/session.py", line 70, in wrapper
    return func(*args, session=session, **kwargs)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/providers/amazon/aws/operators/ecs.py", line 218, in execute
    self._try_reattach_task()
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/providers/amazon/aws/operators/ecs.py", line 309, in _try_reattach_task
    previous_task_arn = self.xcom_pull(
TypeError: xcom_pull() missing 1 required positional argument: 'context'

I updated the unit tests and ran that new code locally to make sure it was working as expected.

@o-nikolas @kaxil

[potiuk]

Ah cool!

[github-actions]

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.

[o-nikolas]

Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided). I'm curious how this got past manual testing though.

[pmalafosse]

Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided). I'm curious how this got past manual testing though.

this happened because in my manual testing I was using a previous version I had locally before a refactor :
context["ti"].xcom_pull(task_ids=..., key=...) to

self.xcom_pull(
            context,
            task_ids=...,
            key=...,
        )

I had to do this refactor to use mocking in the tests

[potiuk]

Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided).

It's not very well understood concept but unit tests do not replace manual tests, not even are a proof that things are right. They are there mostly to make sure that behaviour have not changed and is the same as before (so prevent regression). And having unit tests written at all (whether they are correct or not) makes it simply easier to fix things and re-run tests after we find and fix problems (and fix test afterwards :)).

[kaxil]

Thanks 👏 @pmalafosse

[o-nikolas]

Nice catch, shows how easily mocking in tests can be fraught (i.e. in this case missing the assert that context was provided).

It's not very well understood concept but unit tests do not replace manual tests, not even are a proof that things are right. They are there mostly to make sure that behaviour have not changed and is the same as before (so prevent regression). And having unit tests written at all (whether they are correct or not) makes it simply easier to fix things and re-run tests after we find and fix problems (and fix test afterwards :)).

Totally agree, system/E2E tests are also key for catching these types of things (i.e. functional issues) rather than the quick regression testing that unit tests provide.