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/02/05 21:12:46 UTC
[GitHub] [airflow] pierrejeambrun opened a new pull request #21349: Switch from zdesk to zenpy in ZendeskHook
pierrejeambrun opened a new pull request #21349:
URL: https://github.com/apache/airflow/pull/21349
closes: https://github.com/apache/airflow/issues/8937
This pull request changes `ZendeskHook` to use `zenpy` package instead of `zdesk`.
I tried to introduced as little breaking changes as possible. We keep the ability to make 'cutom' get request via the `call` method.
This is a first draft so we can add dedicated hooks method for each ressource.
Let me know what you think.
--
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] eladkal commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800217310
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,91 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (alternative to old ZendeskHook.call method)
+ self.get = self.zenpy_client.users._get
Review comment:
users may not be familiar with the old lib.
Here we should just explain what this parameter is for (or preferably in example dag)
The explanation about backward compatibility is already written in the change log
--
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] potiuk commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030917923
Just docker-tests failing (which we need to fix). merging.
--
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] pierrejeambrun commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030871635
@eladkal Thank you for your review.
I updated the PR accordingly and added the `ticket` resource as an example. For now the hook is just a wrapper around `zenpy` and is not doing much but we could imagine more sophisticated use case.
--
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] pierrejeambrun commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800222875
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,91 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (alternative to old ZendeskHook.call method)
+ self.get = self.zenpy_client.users._get
Review comment:
Yes no worries. I have removed this comment and added a simple example DAG to illustrate this. (also added the `example DAGs` link to the doc)
--
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] potiuk commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030909343
You should also update provider.yaml for zendesk to set the 3.0.0 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
[GitHub] [airflow] pierrejeambrun commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800206149
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,64 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (ZendeskHook.call method)
+ self._get = self.zenpy_client.users._get
+
+ def _init_conn(self) -> Tuple[Zenpy, str]:
+ """
+ Create the Zenpy Client for our Zendesk connection.
- def get_conn(self) -> Zendesk:
- conn = self.get_connection(self.__zendesk_conn_id)
- self.__url = "https://" + conn.host
- return Zendesk(
- zdesk_url=self.__url, zdesk_email=conn.login, zdesk_password=conn.password, zdesk_token=True
- )
+ :return: zenpy.Zenpy client and the url for the API.
+ """
+ conn = self.get_connection(self.zendesk_conn_id)
+ url = "https://" + conn.host
+ domain = conn.host
+ subdomain: Optional[str] = None
+ if conn.host.count(".") >= 2:
+ dot_splitted_string = conn.host.rsplit(".", 2)
+ subdomain = dot_splitted_string[0]
+ domain = ".".join(dot_splitted_string[1:])
+ return Zenpy(domain=domain, subdomain=subdomain, email=conn.login, password=conn.password), url
- def __handle_rate_limit_exception(self, rate_limit_exception: ZendeskError) -> None:
+ def get_conn(self) -> Zenpy:
"""
- Sleep for the time specified in the exception. If not specified, wait
- for 60 seconds.
+ Get the underlying Zenpy client.
+
+ :return: zenpy.Zenpy client.
"""
- retry_after = int(rate_limit_exception.response.headers.get('Retry-After', 60))
- self.log.info("Hit Zendesk API rate limit. Pausing for %s seconds", retry_after)
- time.sleep(retry_after)
+ return self.zenpy_client
def call(
self,
path: str,
query: Optional[dict] = None,
- get_all_pages: bool = True,
- side_loading: bool = False,
- ) -> dict:
+ **kwargs: Any,
+ ) -> BaseObject:
"""
- Call Zendesk API and return results
+ Call Zendesk API and return results.
- :param path: The Zendesk API to call
- :param query: Query parameters
- :param get_all_pages: Accumulate results over all pages before
- returning. Due to strict rate limiting, this can often timeout.
- Waits for recommended period between tries after a timeout.
- :param side_loading: Retrieve related records as part of a single
- request. In order to enable side-loading, add an 'include'
- query parameter containing a comma-separated list of resources
- to load. For more information on side-loading see
- https://developer.zendesk.com/rest_api/docs/core/side_loading
- """
- query_params = query or {}
- zendesk = self.get_conn()
- first_request_successful = False
-
- while not first_request_successful:
- try:
- results = zendesk.call(path, query_params)
- first_request_successful = True
- except RateLimitError as rle:
- self.__handle_rate_limit_exception(rle)
-
- # Find the key with the results
- keys = [path.split("/")[-1].split(".json")[0]]
- next_page = results['next_page']
- if side_loading:
- keys += query_params['include'].split(',')
- results = {key: results[key] for key in keys}
+ This endpoint is kept for backward compatibility purpose but it should be
+ removed in the future to expose specific methods for each resource.
+ TODO: When removing this method, remove the reference to _get in __init__
Review comment:
Done
--
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] eladkal commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800143111
##########
File path: airflow/providers/zendesk/CHANGELOG.rst
##########
@@ -19,6 +19,19 @@
Changelog
---------
+2.0.2
Review comment:
```suggestion
3.0.0
```
Since this is a breaking change
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,64 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (ZendeskHook.call method)
+ self._get = self.zenpy_client.users._get
Review comment:
This is a private var so users should not really relay on it.
If we want to make this functionality available we should expose it as public
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,64 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (ZendeskHook.call method)
+ self._get = self.zenpy_client.users._get
+
+ def _init_conn(self) -> Tuple[Zenpy, str]:
+ """
+ Create the Zenpy Client for our Zendesk connection.
- def get_conn(self) -> Zendesk:
- conn = self.get_connection(self.__zendesk_conn_id)
- self.__url = "https://" + conn.host
- return Zendesk(
- zdesk_url=self.__url, zdesk_email=conn.login, zdesk_password=conn.password, zdesk_token=True
- )
+ :return: zenpy.Zenpy client and the url for the API.
+ """
+ conn = self.get_connection(self.zendesk_conn_id)
+ url = "https://" + conn.host
+ domain = conn.host
+ subdomain: Optional[str] = None
+ if conn.host.count(".") >= 2:
+ dot_splitted_string = conn.host.rsplit(".", 2)
+ subdomain = dot_splitted_string[0]
+ domain = ".".join(dot_splitted_string[1:])
+ return Zenpy(domain=domain, subdomain=subdomain, email=conn.login, password=conn.password), url
- def __handle_rate_limit_exception(self, rate_limit_exception: ZendeskError) -> None:
+ def get_conn(self) -> Zenpy:
"""
- Sleep for the time specified in the exception. If not specified, wait
- for 60 seconds.
+ Get the underlying Zenpy client.
+
+ :return: zenpy.Zenpy client.
"""
- retry_after = int(rate_limit_exception.response.headers.get('Retry-After', 60))
- self.log.info("Hit Zendesk API rate limit. Pausing for %s seconds", retry_after)
- time.sleep(retry_after)
+ return self.zenpy_client
def call(
self,
path: str,
query: Optional[dict] = None,
- get_all_pages: bool = True,
- side_loading: bool = False,
- ) -> dict:
+ **kwargs: Any,
+ ) -> BaseObject:
"""
- Call Zendesk API and return results
+ Call Zendesk API and return results.
- :param path: The Zendesk API to call
- :param query: Query parameters
- :param get_all_pages: Accumulate results over all pages before
- returning. Due to strict rate limiting, this can often timeout.
- Waits for recommended period between tries after a timeout.
- :param side_loading: Retrieve related records as part of a single
- request. In order to enable side-loading, add an 'include'
- query parameter containing a comma-separated list of resources
- to load. For more information on side-loading see
- https://developer.zendesk.com/rest_api/docs/core/side_loading
- """
- query_params = query or {}
- zendesk = self.get_conn()
- first_request_successful = False
-
- while not first_request_successful:
- try:
- results = zendesk.call(path, query_params)
- first_request_successful = True
- except RateLimitError as rle:
- self.__handle_rate_limit_exception(rle)
-
- # Find the key with the results
- keys = [path.split("/")[-1].split(".json")[0]]
- next_page = results['next_page']
- if side_loading:
- keys += query_params['include'].split(',')
- results = {key: results[key] for key in keys}
+ This endpoint is kept for backward compatibility purpose but it should be
+ removed in the future to expose specific methods for each resource.
+ TODO: When removing this method, remove the reference to _get in __init__
Review comment:
I agree. As mentioned before this is a private function so we can remove it.
Can you add a single resource? It would be easier for others to add more methods when the first one available.
--
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] pierrejeambrun commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800206167
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,64 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (ZendeskHook.call method)
+ self._get = self.zenpy_client.users._get
Review comment:
Done
##########
File path: airflow/providers/zendesk/CHANGELOG.rst
##########
@@ -19,6 +19,19 @@
Changelog
---------
+2.0.2
Review comment:
Done
--
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] github-actions[bot] commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030906680
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.
--
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] pierrejeambrun commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030910216
@potiuk thank you, I just updated 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] eladkal commented on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030923778
thanks @pierrejeambrun !
--
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] pierrejeambrun commented on a change in pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on a change in pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#discussion_r800222875
##########
File path: airflow/providers/zendesk/hooks/zendesk.py
##########
@@ -31,90 +32,91 @@ class ZendeskHook(BaseHook):
:param zendesk_conn_id: The Airflow connection used for Zendesk credentials.
"""
- def __init__(self, zendesk_conn_id: str) -> None:
+ conn_name_attr = 'zendesk_conn_id'
+ default_conn_name = 'zendesk_default'
+ conn_type = 'zendesk'
+ hook_name = 'Zendesk'
+
+ def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
- self.__zendesk_conn_id = zendesk_conn_id
- self.__url = None
+ self.zendesk_conn_id = zendesk_conn_id
+ self.base_api: Optional[BaseApi] = None
+ zenpy_client, url = self._init_conn()
+ self.zenpy_client = zenpy_client
+ self.__url = url
+ # Keep reference to the _get method to allow arbitrary endpoint call for
+ # backwards compatibility. (alternative to old ZendeskHook.call method)
+ self.get = self.zenpy_client.users._get
Review comment:
Ok great. I have removed this comment and added a simple example DAG to illustrate this. (also added the `example DAGs` link to the doc)
--
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] potiuk merged pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21349:
URL: https://github.com/apache/airflow/pull/21349
--
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] pierrejeambrun edited a comment on pull request #21349: Switch from zdesk to zenpy in ZendeskHook
Posted by GitBox <gi...@apache.org>.
pierrejeambrun edited a comment on pull request #21349:
URL: https://github.com/apache/airflow/pull/21349#issuecomment-1030871635
@eladkal Thank you for your review.
I updated the PR accordingly and added the `ticket` resource as an example. I tested all the methods, it's working fine.
For now the hook is just a wrapper around `zenpy` and is not doing much but we could imagine more sophisticated use case.
--
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