Adding temporary python operator imports to compat

[gopidesupavan]

Am trying to add imports in compat file but, ci is replacing these files. so adding temporary imports and once it merged this help tests to run in this #42081.

failing to resolve:
https://github.com/apache/airflow/actions/runs/11307601726/job/31449600261#step:11:2982

---

^ 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.

[potiuk]

No. That is generally bad idea to have tests_common in dev (and this case shows why).

Dev is indeed replaced in CI when images - in "checkout target commit":

        echo
        echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m"
        echo
        rm -rfv "scripts/ci"
        rm -rfv "dev"
        rm -rfv ".github/actions"
        rm -rfv ".github/workflows"
        mv -v "target-airflow/scripts/ci" "scripts"
        mv -v "target-airflow/dev" "."
        mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github"

This is for security reason, so that any of the "dev" scripts that are used during CI and build process are not tampered with when they are modified in the incoming PR.

This is a bit of a teething problem after #42505 and you were the unlucky first non-committer who attempted to modify common test code that is shared between providers and airflow.

@ashb - we will have to find better place for "tests_common" than dev - because otherwise every non-commiter who wants to make a PR changing the "common" code they will get their "dev/tests_common" code overwritten.

Generally "dev" was always supposed to be used for "build scripts" but never by "test code".

How about just "tests_common" at the top level of the repository @ashb ?

[gopidesupavan]

No. That is generally bad idea to have tests_common in dev (and this case shows why).

Dev is indeed replaced in CI when images - in "checkout target commit":

        echo
        echo -e "\033[33m Replace scripts, dev, actions with target branch for non-committer builds!\033[0m"
        echo
        rm -rfv "scripts/ci"
        rm -rfv "dev"
        rm -rfv ".github/actions"
        rm -rfv ".github/workflows"
        mv -v "target-airflow/scripts/ci" "scripts"
        mv -v "target-airflow/dev" "."
        mv -v "target-airflow/.github/actions" "target-airflow/.github/workflows" ".github"

This is for security reason, so that any of the "dev" scripts that are used during CI and build process are not tampered with when they are modified in the incoming PR.

This is a bit of a teething problem after #42505 and you were the unlucky first non-committer who attempted to modify common test code that is shared between providers and airflow.

@ashb - we will have to find better place for "tests_common" than dev - because otherwise every non-commiter who wants to make a PR changing the "common" code they will get their "dev/tests_common" code overwritten.

Generally "dev" was always supposed to be used for "build scripts" but never by "test code".

How about just "tests_common" at the top level of the repository @ashb ?

Thanks jarek I agree , sorry for this :( previously it was in different location. So thought might be because of restructure it moved...

[gopidesupavan]

oops sorry have closed this, conversation is still open here. opening this pr, but merge is not required.

[ashb]

The reason I picked dev/ was cos I was trying to counter/not add to the proliferation of top level folders (there are more than I want/think there should be still). But I see that causes a problem now

[ashb]

In this specific case does the compat import need to be shared between core and provider tests?

I.e. yes dev/tests_common is the wrong place, but I'm not sure if we need import compat in the tests code do we? What is stopping us updating all the imports in tests? @gopidesupavan

[ashb]

@potiuk could we (safely) change dev/ to dev/breeze/ in that target pr part? I'm not sure what else actually lives in dev folder and am not at my computer right now

[potiuk]

@potiuk could we (safely) change dev/ to dev/breeze/ in that target pr part? I'm not sure what else actually lives in dev folder and am not at my computer right now

Not really - dev is really a bag-of-all-things related to building our software - and it's a dangerous thing to only override part of it. There are a number of scripts that are there that are likely to be used in CI processes. This is for example why we have "scripts/ci" - because there I made sure (and I am reviewing that) that only CI scripts are used there).

The current content of dev is this (so there are a number of sub-folders - breeze is only one of them, and it's very likely some other scripts (provider_packages for example) are "potentially dangerous":

chart
example_dags
images
mypy
perf
provider_packages
stats
system_tests
templates
__init__.py
airflow-github
airflow-license
assign_cherry_picked_prs_with_milestone.py
check_files.py
CHERRY_PICK_SUMMARY.txt.jinja2
get_devel_deps.sh
ISSUE_TEMPLATE.md.jinja2
MANUALLY_BUILDING_IMAGES.md
MANUALLY_GENERATING_IMAGE_CACHE_AND_CONSTRAINTS.md
prepare_bulk_issues.py
PROJECT_GUIDELINES.md
PROVIDER_PACKAGE_DETAILS.md
README.md
README_RELEASE_AIRFLOW.md
README_RELEASE_HELM_CHART.md
README_RELEASE_PROVIDER_PACKAGES.md
README_RELEASE_PYTHON_CLIENT.md
refresh_images.sh
remove_artifacts.sh
requirements.txt
retag_docker_images.py
send_email.py
sign.sh
validate_version_added_fields_in_config.py
WHAT_GOES_INTO_THE_NEXT_RELEASE.md

And for example "system_tests" there is not something that "system_tests" use but some "dev/build scripts" that are related to system_tests.

We already had the issue when accidental typo or mistake cause https://medium.com/apache-airflow/unraveling-the-code-navigating-a-ci-release-security-vulnerability-in-apache-airflow-620214a96297 so I think it's better to be safe than sorry and exclude whole "dev" and designate it as "build scripts".

Conceptually - if we look at that the "common_tests" belong to "application" not to "building appplication".

Ideally we should have something like that:

airflow_core/src
                airflow.<subpackages>
             tests
                airflow.<subpackages>
             pyproject.toml
airflow_common/
              tests_common.<subpackages>
              pyproject.toml
providers/
        airbyte/
              src
                 airflow.providers.arbyte.<subpackages> 
              tests
                 airflow.providers.arbyte.<subpackages> 
              pyproject.toml

But it would require the "heavy" airflow move. And maybe we can do it eventually after we split providers and go through all other teething problems.

But maybe let's do it step-by-step - moving to tests_common at top level would be a good intermediate step.

[ashb]

Yeah I thought they was likely the case but wanted to check.

I do plan on eventually moving airflow/ and tests/ into a new "core" workspace project, but absolutely one thing at a time.

[gopidesupavan]

In this specific case does the compat import need to be shared between core and provider tests?

I.e. yes dev/tests_common is the wrong place, but I'm not sure if we need import compat in the tests code do we? What is stopping us updating all the imports in tests? @gopidesupavan

Yeah currently its the pattern i see in the providers tests/ other places. adding imports in compat and using if any compatibility requires.

At present i have workaround like this where ever i need to import doing this.

try:
    from airflow.providers.standard.operators.python import PythonOperator
except ImportError:
    from airflow.operators.python import PythonOperator

This pr update not super important, as have workaround to proceed python operator :)

[potiuk]

The PR to move tests_common in #42985.

[potiuk]

try:
from airflow.providers.standard.operators.python import PythonOperator
except ImportError:
from airflow.operators.python import PythonOperator

Yeah. that pattern is fine - just "dev" override is problematic and hopefully we will fix it soon with #42985

[potiuk]

You should be able to rebase your changes @gopidesupavan now and it should work normaly.

[gopidesupavan]

Closing this one as it resolved #42985