Skip to content

Move docker decorator example dag to docker provider - #18739

Merged
kaxil merged 2 commits into
apache:mainfrom
astronomer:move-dockerexample
Oct 6, 2021
Merged

Move docker decorator example dag to docker provider#18739
kaxil merged 2 commits into
apache:mainfrom
astronomer:move-dockerexample

Conversation

@ephraimbuddy

Copy link
Copy Markdown
Contributor

This example dag errors out during startup when we set AIRFLOW__CORE__EXAMPLE_DAGS=True and docker
provider is not installed.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@kaxil kaxil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should leave the virtualenv task here and separate out the docker task into a separate example DAG in it's provider

@ephraimbuddy
ephraimbuddy force-pushed the move-dockerexample branch 2 times, most recently from 2e8e313 to e17e1be Compare October 5, 2021 15:11
@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 5, 2021
@github-actions

github-actions Bot commented Oct 5, 2021

Copy link
Copy Markdown
Contributor

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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil

kaxil commented Oct 5, 2021

Copy link
Copy Markdown
Member

@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

Hi @potiuk , this failure seems related to DockerDecorator not usable in airflow versions less than 2.2.
It is failing at import_all_provider_classes on airflow 2.1 for install from wheel but passing for sdist

@potiuk

potiuk commented Oct 6, 2021

Copy link
Copy Markdown
Member

Hi @potiuk , this failure seems related to DockerDecorator not usable in airflow versions less than 2.2. It is failing at import_all_provider_classes on airflow 2.1 for install from wheel but passing for sdist

Yep. This is precisely what is expected. The problem is that we try if "ALL" provider code imports nicely for 2.1 without errors - including the examples. In this case we've made a deliberate decision to move the example to "docker" provider, even if it will not work for 2.1. This is fine, so the solution is to try/except the example to get example_dag import without the exception (with appropriate comment).

This example dag errors out during startup when we set `AIRFLOW__CORE__EXAMPLE_DAGS=True` and docker
provider is not installed.

separate the examples

remove doc text since it's not used in documentation
@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

Hi @potiuk , this failure seems related to DockerDecorator not usable in airflow versions less than 2.2. It is failing at import_all_provider_classes on airflow 2.1 for install from wheel but passing for sdist

Yep. This is precisely what is expected. The problem is that we try if "ALL" provider code imports nicely for 2.1 without errors - including the examples. In this case we've made a deliberate decision to move the example to "docker" provider, even if it will not work for 2.1. This is fine, so the solution is to try/except the example to get example_dag import without the exception (with appropriate comment).

Done. Please take a look

@kaxil
kaxil merged commit 43f334f into apache:main Oct 6, 2021
@kaxil
kaxil deleted the move-dockerexample branch October 6, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants