You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/15 06:10:35 UTC
[GitHub] [airflow] dstandish opened a new pull request, #27070: Allow and prefer non-prefixed extra fields for slack hooks
dstandish opened a new pull request, #27070:
URL: https://github.com/apache/airflow/pull/27070
From airflow version 2.3, extra prefixes are not required so we enable them here.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r996578147
##########
airflow/providers/slack/hooks/slack.py:
##########
@@ -28,14 +29,45 @@
from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException, AirflowNotFoundException
from airflow.hooks.base import BaseHook
-from airflow.providers.slack.utils import ConnectionExtraConfig, prefixed_extra_field
+from airflow.providers.slack.utils import ConnectionExtraConfig
from airflow.utils.log.secrets_masker import mask_secret
if TYPE_CHECKING:
from slack_sdk.http_retry import RetryHandler
from slack_sdk.web.slack_response import SlackResponse
+def _ensure_prefixes(conn_type):
+ """
+ Remove when provider min airflow version >= 2.5.0 since this is handled by
+ provider manager from that version.
+ """
+
+ def dec(func):
+ def _ensure_prefix_for_placeholders(field_behaviors: dict[str, Any], conn_type: str):
+ conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'}
+
+ def _ensure_prefix(field):
+ if field not in conn_attrs and not field.startswith('extra__'):
+ return f"extra__{conn_type}__{field}"
+ else:
+ return field
+
+ if 'placeholders' in field_behaviors:
+ placeholders = field_behaviors['placeholders']
+ field_behaviors['placeholders'] = {_ensure_prefix(k): v for k, v in placeholders.items()}
+
+ return field_behaviors
+
+ @wraps(func)
+ def inner(cls):
+ return _ensure_prefix_for_placeholders(func(cls), conn_type)
Review Comment:
Is this simply…
```suggestion
@wraps(func)
def inner(cls):
field_behaviors = func(cls)
conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'}
def _ensure_prefix(field):
if field not in conn_attrs and not field.startswith('extra__'):
return f"extra__{conn_type}__{field}"
else:
return field
if 'placeholders' in field_behaviors:
placeholders = field_behaviors['placeholders']
field_behaviors['placeholders'] = {_ensure_prefix(k): v for k, v in placeholders.items()}
return field_behaviors
```
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r1001045567
##########
airflow/providers/slack/hooks/slack_webhook.py:
##########
@@ -55,6 +55,34 @@ def wrapper(*args, **kwargs) -> Callable:
return wrapper
+def _ensure_prefixes(conn_type):
Review Comment:
you are correct. it's not possible because the only thing providers share is airflow, and we can't use features in airflow until they are present in the version specified as dep in provider. the feature that eliminates the need for this decorator will be released in 2.5. when min airflow version for these providers is 2.5, then we can remove this decorator. and if you look at the tests, i actually force this removal at this time! it's tricky managing all the backcompat here but ... that's life!
--
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
[GitHub] [airflow] dstandish merged pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish merged PR #27070:
URL: https://github.com/apache/airflow/pull/27070
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r996579932
##########
airflow/providers/slack/hooks/slack.py:
##########
@@ -28,14 +29,45 @@
from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException, AirflowNotFoundException
from airflow.hooks.base import BaseHook
-from airflow.providers.slack.utils import ConnectionExtraConfig, prefixed_extra_field
+from airflow.providers.slack.utils import ConnectionExtraConfig
from airflow.utils.log.secrets_masker import mask_secret
if TYPE_CHECKING:
from slack_sdk.http_retry import RetryHandler
from slack_sdk.web.slack_response import SlackResponse
+def _ensure_prefixes(conn_type):
+ """
+ Remove when provider min airflow version >= 2.5.0 since this is handled by
+ provider manager from that version.
+ """
+
+ def dec(func):
+ def _ensure_prefix_for_placeholders(field_behaviors: dict[str, Any], conn_type: str):
+ conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'}
+
+ def _ensure_prefix(field):
+ if field not in conn_attrs and not field.startswith('extra__'):
+ return f"extra__{conn_type}__{field}"
+ else:
+ return field
+
+ if 'placeholders' in field_behaviors:
+ placeholders = field_behaviors['placeholders']
+ field_behaviors['placeholders'] = {_ensure_prefix(k): v for k, v in placeholders.items()}
+
+ return field_behaviors
+
+ @wraps(func)
+ def inner(cls):
+ return _ensure_prefix_for_placeholders(func(cls), conn_type)
Review Comment:
yup, looks like it :)
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r996582387
##########
airflow/providers/slack/hooks/slack.py:
##########
@@ -28,14 +29,45 @@
from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException, AirflowNotFoundException
from airflow.hooks.base import BaseHook
-from airflow.providers.slack.utils import ConnectionExtraConfig, prefixed_extra_field
+from airflow.providers.slack.utils import ConnectionExtraConfig
from airflow.utils.log.secrets_masker import mask_secret
if TYPE_CHECKING:
from slack_sdk.http_retry import RetryHandler
from slack_sdk.web.slack_response import SlackResponse
+def _ensure_prefixes(conn_type):
+ """
+ Remove when provider min airflow version >= 2.5.0 since this is handled by
+ provider manager from that version.
+ """
+
+ def dec(func):
+ def _ensure_prefix_for_placeholders(field_behaviors: dict[str, Any], conn_type: str):
+ conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'}
+
+ def _ensure_prefix(field):
+ if field not in conn_attrs and not field.startswith('extra__'):
+ return f"extra__{conn_type}__{field}"
+ else:
+ return field
+
+ if 'placeholders' in field_behaviors:
+ placeholders = field_behaviors['placeholders']
+ field_behaviors['placeholders'] = {_ensure_prefix(k): v for k, v in placeholders.items()}
+
+ return field_behaviors
+
+ @wraps(func)
+ def inner(cls):
+ return _ensure_prefix_for_placeholders(func(cls), conn_type)
Review Comment:
although "simply" may be overselling it 😜
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r996585897
##########
airflow/providers/slack/hooks/slack.py:
##########
@@ -28,14 +29,45 @@
from airflow.compat.functools import cached_property
from airflow.exceptions import AirflowException, AirflowNotFoundException
from airflow.hooks.base import BaseHook
-from airflow.providers.slack.utils import ConnectionExtraConfig, prefixed_extra_field
+from airflow.providers.slack.utils import ConnectionExtraConfig
from airflow.utils.log.secrets_masker import mask_secret
if TYPE_CHECKING:
from slack_sdk.http_retry import RetryHandler
from slack_sdk.web.slack_response import SlackResponse
+def _ensure_prefixes(conn_type):
+ """
+ Remove when provider min airflow version >= 2.5.0 since this is handled by
+ provider manager from that version.
+ """
+
+ def dec(func):
+ def _ensure_prefix_for_placeholders(field_behaviors: dict[str, Any], conn_type: str):
+ conn_attrs = {'host', 'schema', 'login', 'password', 'port', 'extra'}
+
+ def _ensure_prefix(field):
+ if field not in conn_attrs and not field.startswith('extra__'):
+ return f"extra__{conn_type}__{field}"
+ else:
+ return field
+
+ if 'placeholders' in field_behaviors:
+ placeholders = field_behaviors['placeholders']
+ field_behaviors['placeholders'] = {_ensure_prefix(k): v for k, v in placeholders.items()}
+
+ return field_behaviors
+
+ @wraps(func)
+ def inner(cls):
+ return _ensure_prefix_for_placeholders(func(cls), conn_type)
Review Comment:
ok... i applied this to all branches
--
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
[GitHub] [airflow] vincbeck commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
vincbeck commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r1001030978
##########
airflow/providers/slack/hooks/slack_webhook.py:
##########
@@ -55,6 +55,34 @@ def wrapper(*args, **kwargs) -> Callable:
return wrapper
+def _ensure_prefixes(conn_type):
Review Comment:
I can see this function is a duplicate of the one defined in `airflow/providers/slack/hooks/slack.py` (and in many other PR you have open). Would not it be better to put it somewhere common once and then use it across the different providers?
--
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
[GitHub] [airflow] vincbeck commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
vincbeck commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r1001088526
##########
airflow/providers/slack/hooks/slack_webhook.py:
##########
@@ -55,6 +55,34 @@ def wrapper(*args, **kwargs) -> Callable:
return wrapper
+def _ensure_prefixes(conn_type):
Review Comment:
I see. Could you then at least try to have it only once per provider? Here (Slack), you defined this function twice
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #27070: Allow and prefer non-prefixed extra fields for slack hooks
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27070:
URL: https://github.com/apache/airflow/pull/27070#discussion_r1001143379
##########
airflow/providers/slack/hooks/slack_webhook.py:
##########
@@ -55,6 +55,34 @@ def wrapper(*args, **kwargs) -> Callable:
return wrapper
+def _ensure_prefixes(conn_type):
Review Comment:
oy, sure
--
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