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/03/04 15:13:09 UTC
[GitHub] [airflow] frankcash opened a new pull request #21997: [Feature] Updates FTPHook provider to have test_connection
frankcash opened a new pull request #21997:
URL: https://github.com/apache/airflow/pull/21997
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Issue: https://github.com/apache/airflow/issues/21996
Related: https://github.com/apache/airflow/pull/19609
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#discussion_r820138990
##########
File path: tests/providers/ftp/hooks/test_ftp.py
##########
@@ -120,6 +120,19 @@ def test_retrieve_file_with_callback(self):
ftp_hook.retrieve_file(self.path, _buffer, callback=func)
self.conn_mock.retrbinary.assert_called_once_with('RETR path', func)
+ def test_connection_success(self):
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is True
+ assert msg == 'Connection successfully tested'
+
Review comment:
```suggestion
```
--
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] frankcash commented on a change in pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
frankcash commented on a change in pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#discussion_r820152092
##########
File path: tests/providers/ftp/hooks/test_ftp.py
##########
@@ -120,6 +120,19 @@ def test_retrieve_file_with_callback(self):
ftp_hook.retrieve_file(self.path, _buffer, callback=func)
self.conn_mock.retrbinary.assert_called_once_with('RETR path', func)
+ def test_connection_success(self):
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is True
+ assert msg == 'Connection successfully tested'
+
+ def test_connection_failure(self):
+ self.conn_mock = mock.MagicMock(name='conn_mock', side_effect=Exception('Test'))
+ fh.FTPHook.get_conn = self.conn_mock
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is False
+ assert msg == 'Test'
Review comment:
`Suggestion cannot be identical to original text. `
--
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] frankcash commented on pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
frankcash commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059963451
Thanks @eladkal
--
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] frankcash commented on pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
frankcash commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059766352
@josh-fell thanks for the feedback, I will definitely create a situation where it would return connection failed.
As far as testing the `FTPSHook()` I think is a bit out of scope of what I aimed to do
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059911359
I rebased and applied a fix to the static checks.. lets see if it helps
--
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] frankcash closed pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
frankcash closed pull request #21997:
URL: https://github.com/apache/airflow/pull/21997
--
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] josh-fell edited a comment on pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
josh-fell edited a comment on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059615713
It would be a good idea to add a `test_connection_failed` test as well.
Also, it doesn't need to be in this PR but feel free to add if you feel up to it, but adding tests for `test_connection()` for the `FTPSHook` too especially since it inherits from `FTPHook` and overwrites `get_conn()` (which is used in `FTPHook.test_connection()`). WDYT?
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059974880
A little mishap with this one as it broke the main tests (but it is no-one's fault :) #22026 fixes it and prevents similar problems like that in the future)
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#discussion_r820138511
##########
File path: tests/providers/ftp/hooks/test_ftp.py
##########
@@ -120,6 +120,19 @@ def test_retrieve_file_with_callback(self):
ftp_hook.retrieve_file(self.path, _buffer, callback=func)
self.conn_mock.retrbinary.assert_called_once_with('RETR path', func)
+ def test_connection_success(self):
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is True
+ assert msg == 'Connection successfully tested'
+
+ def test_connection_failure(self):
+ self.conn_mock = mock.MagicMock(name='conn_mock', side_effect=Exception('Test'))
+ fh.FTPHook.get_conn = self.conn_mock
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is False
+ assert msg == 'Test'
Review comment:
```suggestion
assert msg == 'Test'
```
--
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] josh-fell commented on pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059615713
It would be a good idea to add a `test_connection_failed` test as well.
Also, it doesn't need to be in this PR but feel free to add if you feel up to it, to also add tests for `test_connection()` for the `FTPSHook` too especially since it inherits from `FTPHook` and overwrites `get_conn()` (which is used in `FTPHook.test_connection()`). WDYT?
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059775473
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main 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] eladkal commented on a change in pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#discussion_r820132049
##########
File path: tests/providers/ftp/hooks/test_ftp.py
##########
@@ -120,6 +120,20 @@ def test_retrieve_file_with_callback(self):
ftp_hook.retrieve_file(self.path, _buffer, callback=func)
self.conn_mock.retrbinary.assert_called_once_with('RETR path', func)
+ def test_connection_success(self):
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is True
+ assert msg == 'Connection successfully tested'
+
+ def test_connection_failure(self):
+ self.conn_mock = mock.MagicMock(name='conn_mock', side_effect=Exception('Test'))
+ fh.FTPHook.get_conn = self.conn_mock
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is False
+ assert msg == 'Test'
+
Review comment:
```suggestion
```
to fix static checks
--
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] frankcash commented on pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
frankcash commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059842506
@eladkal I merged the changes I could, but it appears to still be failing the static checks
--
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 merged pull request #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #21997:
URL: https://github.com/apache/airflow/pull/21997
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#issuecomment-1059978629
> A little mishap with this one as it broke the main tests (but it is no-one's fault :) #22026 fixes it and prevents similar problems like that in the future)
ahhhh
--
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 #21997: [Feature] Updates FTPHook provider to have test_connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #21997:
URL: https://github.com/apache/airflow/pull/21997#discussion_r820138511
##########
File path: tests/providers/ftp/hooks/test_ftp.py
##########
@@ -120,6 +120,19 @@ def test_retrieve_file_with_callback(self):
ftp_hook.retrieve_file(self.path, _buffer, callback=func)
self.conn_mock.retrbinary.assert_called_once_with('RETR path', func)
+ def test_connection_success(self):
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is True
+ assert msg == 'Connection successfully tested'
+
+ def test_connection_failure(self):
+ self.conn_mock = mock.MagicMock(name='conn_mock', side_effect=Exception('Test'))
+ fh.FTPHook.get_conn = self.conn_mock
+ with fh.FTPHook() as ftp_hook:
+ status, msg = ftp_hook.test_connection()
+ assert status is False
+ assert msg == 'Test'
Review comment:
```suggestion
assert msg == 'Test'
```
to fix the static checks
--
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