Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standard provider bash operator #42252

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

gopidesupavan
Copy link
Contributor

Moving bash operators/sensors to standard provider


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

@gopidesupavan
Copy link
Contributor Author

moving this bash operator to standard provider, has module path changes in systems test, integration tests.

failures in python client tests, i am thinking due to bashoperator path change, causing dag not loaded.
Checking remaining tests.

@@ -553,7 +553,19 @@ repos:
^airflow/example_dags/example_sensors.py|
^airflow/example_dags/example_sensors.py|
^airflow/example_dags/example_sensors.py|
^airflow/example_dags/example_time_delta_sensor_async.py
^airflow/example_dags/example_time_delta_sensor_async.py|
Copy link
Contributor

Choose a reason for hiding this comment

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

For this change - if we basically add all example DAGs into the exclude list - the check has almost no effect.

Can you re-work the check such that no provider except standard provider dependencies are accepted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agree. will check.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This all looks a bit strange now in my eyes. All example DAGs sitting in the core are requiring a provider package. Smells like a cyclomatic package dependency.

  1. Should we move the example DAGs into the provider as well? (Then they are hard to find :-( )
  2. Should we load the core operators from the compatability alias? (Then we have bad examples)
  3. Any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This was an open question to the community :-D

Copy link
Member

Choose a reason for hiding this comment

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

  1. Seems reasonable to me. Not that hard to find. But load_examples probably needs more work then. Probably not be worth it.
  2. No.
  3. Leave it? Airflow 3 will require the standard provider anyways right?

Side note: We don't need the try/except fallback imo.

Copy link
Member

Choose a reason for hiding this comment

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

What do we do for the other bundled providers - http, sqlite (plus two others - ftp and email)?

@@ -42,6 +42,7 @@ operators:
python-modules:
- airflow.providers.standard.time.operators.datetime
- airflow.providers.standard.time.operators.weekday
- airflow.providers.standard.core.operators.bash
Copy link
Member

Choose a reason for hiding this comment

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

My 2c, I'm really not sure we need the time/core part of this. Seems better to just have

airflow.providers.standard.operators.weekday
airflow.providers.standard.operators.bash

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 am ok with suggestion. :)

@gopidesupavan
Copy link
Contributor Author

Could you please help on this, kubernetes executor tests are failing with dag not found example_kubernetes_executor and example_bash_operator. one option i tried copying the standard package to k8s build container, but its still failing with same reason.

@gopidesupavan
Copy link
Contributor Author

Additional prod image tests also failing, as it doesnt have the standard provider. inside latest prod image pull.
image

@gopidesupavan
Copy link
Contributor Author

Additional prod image tests also failing, as it doesnt have the standard provider. inside latest prod image pull. image

Any suggestions for these tests?

@gopidesupavan
Copy link
Contributor Author

gopidesupavan commented Sep 21, 2024

After reviewing the logs, it seems the reason the k8s tests are failing is due to the absence of the standard provider package in the image. This is because i guess the standard provider not included in the prod_image_installed_providers.txt file ?

Is it okay to add standard provider in to that file?

@jscheffl
Copy link
Contributor

After reviewing the logs, it seems the reason the k8s tests are failing is due to the absence of the standard provider package in the image. This is because i guess the standard provider not included in the prod_image_installed_providers.txt file ?

Is it okay to add standard provider in to that file?

Yes. At least from the community discussion we always expected that the new "standard" provider will be a standard install and dependency. So adding to prod image would be the natural consequence. Hope that fixes the problems you had (and sorry for responding late,,,)

@gopidesupavan
Copy link
Contributor Author

After reviewing the logs, it seems the reason the k8s tests are failing is due to the absence of the standard provider package in the image. This is because i guess the standard provider not included in the prod_image_installed_providers.txt file ?
Is it okay to add standard provider in to that file?

Yes. At least from the community discussion we always expected that the new "standard" provider will be a standard install and dependency. So adding to prod image would be the natural consequence. Hope that fixes the problems you had (and sorry for responding late,,,)

Thank you :) , I am glad the debug helped me to understand how the ci workflow setups and working process.

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.

5 participants