[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-02-22 Thread via GitHub 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1115118926


##
airflow/providers/telegram/provider.yaml:
##
@@ -39,7 +40,7 @@ dependencies:
   # The telegram bot 20.0.0 is not yet compatible with our provider as 
documented in
   # https://github.com/apache/airflow/issues/28670. This limit should be 
removed (and likely replaced
   # with >=20.0.0) once the issue is fixed.
-  - python-telegram-bot>=13.0,<20.0.0
+  - python-telegram-bot>=20.0.0

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-unsubscr...@airflow.apache.org

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

[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-01-16 Thread GitBox 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1071365785


##
airflow/providers/telegram/CHANGELOG.rst:
##
@@ -24,6 +24,19 @@
 Changelog
 -
 
+4.0.0
+.
+
+Breaking changes
+
+
+* This release of provider only supports ``python-telegram-bot 20.0.0`` and 
above. All previous versions will
+  not work with ``4.0.0``.

Review Comment:
   Ah yes🤦🏻‍♂️ Absolutely.



-- 
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-unsubscr...@airflow.apache.org

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

[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-01-16 Thread GitBox 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1071210603


##
tests/providers/telegram/hooks/test_telegram.py:
##
@@ -128,7 +128,7 @@ def 
test_should_send_message_if_chat_id_is_provided_through_constructor(self, mo
 
 
@mock.patch("airflow.providers.telegram.hooks.telegram.TelegramHook.get_conn")
 def test_should_send_message_if_chat_id_is_provided_in_connection(self, 
mock_get_conn):
-mock_get_conn.return_value = mock.Mock(password="some_token")
+mock_get_conn.return_value = mock.AsyncMock(password="some_token")

Review Comment:
   @Taragolis, yes I noticed, at some point today, that Python 3.7 did not 
contain `AsyncMock`. Should I reuse the implementation in Google Provider? I 
had just added 
[this](https://github.com/apache/airflow/pull/28953/files#diff-186f9ce31911e0f300377123cda4f4c7f310a3dbb48c59ce38f5171457022e8eR35).
 But reusing the implementation in Google Provider is probably a better idea.



-- 
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-unsubscr...@airflow.apache.org

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

[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-01-15 Thread GitBox 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1070914304


##
airflow/providers/telegram/hooks/telegram.py:
##
@@ -137,5 +139,5 @@ def send_message(self, api_params: dict) -> None:
 if kwargs["chat_id"] is None:
 raise AirflowException("'chat_id' must be provided for telegram 
message")
 
-response = self.connection.send_message(**kwargs)
+response = 
asyncio.get_event_loop().run_until_complete(self.connection.send_message(**kwargs))

Review Comment:
   I had initially used `asyncio.run()`, but updated to 
`get_event_loop().run_until_complete()`. Both works



-- 
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-unsubscr...@airflow.apache.org

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

[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-01-15 Thread GitBox 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1070729605


##
tests/providers/telegram/hooks/test_telegram.py:
##
@@ -72,28 +71,31 @@ def 
test_should_raise_exception_if_conn_id_doesnt_contain_token(self):
 
 assert "Missing token(password) in Telegram connection" == 
str(ctx.value)
 
+@pytest.mark.asyncio
 
@mock.patch("airflow.providers.telegram.hooks.telegram.TelegramHook.get_conn")
-def test_should_raise_exception_if_chat_id_is_not_provided_anywhere(self, 
mock_get_conn):
+async def 
test_should_raise_exception_if_chat_id_is_not_provided_anywhere(self, 
mock_get_conn):

Review Comment:
   Updated the hook and removed the need for the `async` methods



-- 
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-unsubscr...@airflow.apache.org

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

[GitHub] [airflow] maxnathaniel commented on a diff in pull request #28953: Updated Telegram Provider to ensure compatbility with >=20.0.0

2023-01-15 Thread GitBox 


maxnathaniel commented on code in PR #28953:
URL: https://github.com/apache/airflow/pull/28953#discussion_r1070729407


##
airflow/providers/telegram/hooks/telegram.py:
##
@@ -118,15 +118,15 @@ def __get_chat_id(self, chat_id: str | None, 
telegram_conn_id: str | None) -> st
 stop=tenacity.stop_after_attempt(5),
 wait=tenacity.wait_fixed(1),
 )
-def send_message(self, api_params: dict) -> None:
+async def send_message(self, api_params: dict) -> None:

Review Comment:
   Thanks for your feedback. My apologies for introducing breaking changes - 
completely missed that users will be calling hook methods in their code. I have 
made the updates as per your recommendation, tested it, and also updated the 
unit test cases.



-- 
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-unsubscr...@airflow.apache.org

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