Add upgrade check rule for unrecognized arguments to Operators #12660

drdenzy · 2020-11-27T16:18:52Z

ease upgrade to Airflow 2.0

The NoAdditionalArgsInOperatorsRule class ensures that passing an
unrecognised arguments to operators causes an exception.

Passing unrecognised arguments is not allowed in Airflow 2.0

fixes #11042

airflow/upgrade/rules/no_additional_args_in_operators.py

turbaszek · 2020-11-27T16:45:21Z

airflow/upgrade/rules/no_additional_args_in_operators.py

If this is here to suppress the log then it won't be necessary after #12657

@Dr-Denzy We can remove the logger.setLevel, and the outer try/finally now, as the PR mentioned here has been merged

airflow/upgrade/rules/no_additional_args_in_operators.py

tests/upgrade/rules/test_no_additional_args_operators.py

turbaszek · 2020-11-27T16:51:25Z

tests/upgrade/rules/test_no_additional_args_operators.py

Suggested change

msgs = rule.check(dags_folder=dag_file.name)

msgs = rule.check()

If we have this argument only for test then we should remove it and use mock similar to this:

airflow/tests/upgrade/rules/test_import_changes.py

Lines 49 to 61 in 847820f

class TestImportChangesRule:

@mock.patch("airflow.upgrade.rules.import_changes.list_py_file_paths")

@mock.patch(

"airflow.upgrade.rules.import_changes.ImportChangesRule.ALL_CHANGES",

[ImportChange.from_new_old_paths(NEW_PATH, OLD_PATH)],

)

def test_check(self, mock_list_files):

with NamedTemporaryFile("w+") as temp:

mock_list_files.return_value = [temp.name]

temp.write("from airflow.contrib import %s" % OLD_CLASS)

temp.flush()

msgs = ImportChangesRule().check()

Ah, the Dependency Injection vs Mocking debate. :)

Having slept on it, I personally find the DI approach easier to understand than mocking.

@ashb personally I don't like features in code that are not used when running the code - it creates confusion when not commented

@turbaszek Since you and I don't agree, and both approaches already exist in the code base, are you happy to let @Dr-Denzy decide on which approach to user?

@turbaszek Since you and I don't agree, and both approaches already exist in the code base, are you happy to let @Dr-Denzy decide on which approach to user?

Yes, no objection to keep it as it is - we already use this approach 👌

airflow/contrib/kubernetes/pod.py

github-actions · 2020-11-30T18:40:40Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

tests/upgrade/rules/test_no_additional_args_operators.py

kaxil · 2020-12-01T22:16:54Z

Weird, passed for me locally too

potiuk · 2020-12-01T22:27:50Z

Weird, passed for me locally too

As discussed here: https://apache-airflow.slack.com/archives/C0146STM600/p1606855518349800 - I belive this might side-effect from another test using the same "test" dag_id. We have a few of those and there are a number of tests that are likely leaving some remnants in the DB. My guess would be, this is the case and cleaning in setUp/tearDown might solve the issue.

turbaszek · 2020-12-02T09:18:37Z

Weird, passed for me locally too

As discussed here: https://apache-airflow.slack.com/archives/C0146STM600/p1606855518349800 - I belive this might side-effect from another test using the same "test" dag_id. We have a few of those and there are a number of tests that are likely leaving some remnants in the DB. My guess would be, this is the case and cleaning in setUp/tearDown might solve the issue.

Sounds familiar. I usually prefer to use tests names as dag/task ids as this make sure that we are not affected by side effects. But cleanup would be also nice.

ease upgrade to Airflow 2.0 The NoAdditionalArgsInOperatorsRule class ensures that passing an unrecognized arguments to operators causes an exception. Passing unrecognized arguments is not allowed in Airflow 2.0

Implemented suggestions made during the PR reviews.

Co-authored-by: Ash Berlin-Taylor <[email protected]>

kaxil · 2020-12-10T18:27:11Z

tests/upgrade/rules/test_no_additional_args_operators.py

This was needed as we suppress DeprecationWarning and PendingDeprecationWarning when running pytest on CI:

Starting the tests with those pytest arguments: --verbosity=0 --strict-markers --durations=100 --cov=airflow/ --cov-config=.coveragerc --cov-report=xml:/files/coverage.xml --color=yes --maxfail=50 --pythonwarnings=ignore::DeprecationWarning --pythonwarnings=ignore::PendingDeprecationWarning --junitxml=/files/test_result.xml --setup-timeout=20 --execution-timeout=60 --teardown-timeout=20 -rfEX --with-db-init tests

Lol took few hours for me & Dennis to figure why we were not able to replicate it when running it with breeze:

root@e6768532ef19:/opt/airflow# pytest tests/upgrade/rules/test_no_additional_args_operators.py

until we ran it with:

root@e6768532ef19:/opt/airflow# pytest tests/upgrade/rules/test_no_additional_args_operators.py --pythonwarnings=ignore::DeprecationWarning --pythonwarnings=ignore::PendingDeprecationWarning

😄

Good work you two!

github-actions · 2020-12-10T19:20:22Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.