[GitHub] [airflow] potiuk commented on a diff in pull request #28569: Add general-purpose "notifier" concept to DAGs

[GitBox <gi...@apache.org>]

potiuk commented on code in PR #28569:
URL: https://github.com/apache/airflow/pull/28569#discussion_r1058359807


##########
airflow/providers/slack/provider.yaml:
##########
@@ -39,7 +39,7 @@ versions:
   - 1.0.0
 
 dependencies:
-  - apache-airflow>=2.3.0
+  - apache-airflow>=2.6.0.dev0

Review Comment:
   > Hi @potiuk, @Taragolis what is the right way to mark this PR for 2.6? Can't find any clue from the error messages
   
   We do not need that . @Taragolis  is right.
   
   > I think this lines in airflow/providers/slack/notifications/slack_notifier.py would be enough, ....
   
   Correct. It should nicely work this way. However we might want to to add special handling in `verify-provider-packages` command:
   
   ```
   breeze release-management verify-provider-packages --use-airflow-version 2.3.0 --use-packages-from-dist --airflow-constraints-reference constraints-2.3.0
   ```
   
   This one is failing now because AirflowOptionalProviderFeatureException is raised:
   
   https://github.com/apache/airflow/actions/runs/3793310830/jobs/6450758471:
   
   ```
   Traceback (most recent call last):
     File "/usr/local/lib/python3.7/site-packages/airflow/providers/slack/notifications/slack_notifier.py", line 26, in <module>
       from airflow.notifications.basenotifier import BaseNotifier
   ModuleNotFoundError: No module named 'airflow.notifications'
   
   During handling of the above exception, another exception occurred:
   
   Traceback (most recent call last):
     File "/opt/airflow/scripts/in_container/verify_providers.py", line 355, in import_all_classes
       _module = importlib.import_module(modinfo.name)
     File "/usr/local/lib/python3.7/importlib/__init__.py", line 127, in import_module
       return _bootstrap._gcd_import(name, package, level)
     File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
     File "<frozen importlib._bootstrap>", line 983, in _find_and_load
     File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
     File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
     File "<frozen importlib._bootstrap_external>", line 728, in exec_module
     File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
     File "/usr/local/lib/python3.7/site-packages/airflow/providers/slack/notifications/slack_notifier.py", line 28, in <module>
       raise AirflowOptionalProviderFeatureException(e)
   airflow.exceptions.AirflowOptionalProviderFeatureException: No module named 'airflow.notifications'
   ```
   
   We need to catch it and ignore. 
   
   We do not have it implemented yet because:
   
   a) it was not possible before because we had Airflow 2.2+
   b) it was not needed because all the "AirflowOptionalProviderFeatureException" were thrown when some libraries were missing only (and we had them all installed in our CI image)
   c) this is the first time we are implementing an optional feature that depends on Airflow version :)
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org