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 2020/09/22 22:35:05 UTC
[GitHub] [airflow] NadimYounes opened a new pull request #11097: Added support for encrypted private keys in SSHHook
NadimYounes opened a new pull request #11097:
URL: https://github.com/apache/airflow/pull/11097
<!--
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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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/master/UPDATING.md).
Hello, I am fairly new to contributing to larger repositories so I want to apologize in advance if there are any mistakes in my Pull Request.
I am submitting a change that will allow the `airflow.providers.ssh.hooks.ssh` to handle encrypted RSA keys. Currently, when the SSHHook attempts to run `get_conn()` on an encrypted RSA key it throws the error `paramiko.PasswordRequiredException: Private key file is encrypted`.
I haven't submitted any tests yet as I just want to make sure that this change is valid and adds value to the airflow community.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes edited a comment on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes edited a comment on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698589998
@XD-DENG Looks like the pylint checks have passed but got a random [connection error on CI Build / Integration:Sqlite Py3.7 ](https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162635344)
The [static check](https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162634545) is still failing on , not sure how to interpret this error. Should I be removing `cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA` from the `tearDownClass`?
Sorry to keep bothering you and thanks a lot for your help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698532008
Hi @NadimYounes . I think the static check is justing putting "too" strict rule on the depth of nested blocks.
I don't favour putting any "ignore" annotation, so the quick fix I can come out with would be to "revoke" the suggestion I have given earlier (https://github.com/apache/airflow/pull/11097#discussion_r493246703). Instead, you go back for your earlier implementation, i.e.
In such a way, the depth would be 3 instead of 4. Then it should be ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697047423
Please rebase as well. Your current parent commit seems quite out-dated (6th July).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698019880
@NadimYounes LGTM. What are your plans for your next contribution? π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697118409
@XD-DENG Rebased, my apologies had an old fork
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697018091
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes closed pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes closed pull request #11097:
URL: https://github.com/apache/airflow/pull/11097
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697117895
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698060015
@mik-laj Working on some other SQLServer/ODBC Related Hooks :) Thanks for promptly reviewing everything..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697018091
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r494660012
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -62,6 +69,7 @@ def tearDownClass(cls) -> None:
with create_session() as session:
conns_to_reset = [
cls.CONN_SSH_WITH_PRIVATE_KEY_EXTRA,
+ cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA
Review comment:
Agreed, the error message wasn't clear. You were spot on though, all checks passed, Cheers!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698519302
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r493925955
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -106,8 +106,11 @@ def __init__(
self.key_file = extra_options.get("key_file")
private_key = extra_options.get('private_key')
- if private_key:
+ private_key_passphrase = extra_options.get('private_key_passphrase')
Review comment:
@XD-DENG Done, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r493240369
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -106,8 +106,11 @@ def __init__(
self.key_file = extra_options.get("key_file")
private_key = extra_options.get('private_key')
- if private_key:
+ private_key_passphrase = extra_options.get('private_key_passphrase')
+ if private_key and not private_key_passphrase:
self.pkey = paramiko.RSAKey.from_private_key(StringIO(private_key))
+ elif private_key and private_key_passphrase:
+ self.pkey = paramiko.RSAKey.from_private_key(StringIO(private_key),password=private_key_passphrase)
Review comment:
Style issue: there should be a space before "password=...", I believe
##########
File path: airflow/providers/ssh/hooks/ssh.py
##########
@@ -106,8 +106,11 @@ def __init__(
self.key_file = extra_options.get("key_file")
private_key = extra_options.get('private_key')
- if private_key:
+ private_key_passphrase = extra_options.get('private_key_passphrase')
Review comment:
Very minor thing (not a must-change): maybe code like below would be more straight forward from reading aspect?
```
if private_key:
if private_key_passphrase:
...
else:
...
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r494614474
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -62,6 +69,7 @@ def tearDownClass(cls) -> None:
with create_session() as session:
conns_to_reset = [
cls.CONN_SSH_WITH_PRIVATE_KEY_EXTRA,
+ cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA
Review comment:
Hi @NadimYounes , may you add a comma at the end of this line? I'm not 100% sure if it's the reason, but worth a try.
I have to say the static check workflow is not working as what I'm expecting, because the error message is not displayed completely (to me).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG merged pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #11097:
URL: https://github.com/apache/airflow/pull/11097
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG merged pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #11097:
URL: https://github.com/apache/airflow/pull/11097
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697047423
Please rebase as well. Your current parent commit seems quite out-dated (6th July).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698593367
Please find my in-line comment above.
Don't have to worry too much about the SQLite error, as you said, it's a random failure (most likely).
And don't worry if this bothers me or not (it does not for sure). More than happy to help, and thanks for your efforts making the contribution!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698721060
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697040381
This change looks good and feels helpful to other people. Could you please add unit tests?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698532008
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698721060
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697117895
> Please rebase as well. Your current parent commit seems quite out-dated (6th July).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698589998
@XD-DENG Looks like the pylint checks have passed but got a random connection error on `CI Build / Integration:Sqlite Py3.7` https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162635344
The static check is still failing on `https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162634545` , not sure how to interpret this error. Should I be removing `cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA` from the `tearDownClass`?
Sorry to keep bothering you and thanks a lot for your help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r494660012
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -62,6 +69,7 @@ def tearDownClass(cls) -> None:
with create_session() as session:
conns_to_reset = [
cls.CONN_SSH_WITH_PRIVATE_KEY_EXTRA,
+ cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA
Review comment:
Agreed, the error message wasn't clear. You were spot on though, all checks passed, Cheers!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes edited a comment on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes edited a comment on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698589998
@XD-DENG Looks like the pylint checks have passed but got a random [connection error on CI Build / Integration:Sqlite Py3.7 ](https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162635344)
The [static check](https://github.com/apache/airflow/pull/11097/checks?check_run_id=1162634545) is still failing on , not sure how to interpret this error. Should I be removing `cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA` from the `tearDownClass`?
Sorry to keep bothering you and thanks a lot for your help.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes closed pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes closed pull request #11097:
URL: https://github.com/apache/airflow/pull/11097
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a change in pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#discussion_r494614474
##########
File path: tests/providers/ssh/hooks/test_ssh.py
##########
@@ -62,6 +69,7 @@ def tearDownClass(cls) -> None:
with create_session() as session:
conns_to_reset = [
cls.CONN_SSH_WITH_PRIVATE_KEY_EXTRA,
+ cls.CONN_SSH_WITH_PRIVATE_KEY_PASSPHRASE_EXTRA
Review comment:
Hi @NadimYounes , may you add a comma at the end of this line? I'm not 100% sure if it's the reason, but worth a try.
I have to say the static check workflow is not working as what I'm expecting, because the error message is not displayed completely (to me).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698519302
@XD-DENG Just finishing up cleaning all the pylint/black highlighting stuff on my local. I am stuck on one last piece and wanted to ask for some advice/recommendation.
`airflow/providers/ssh/hooks/ssh.py:93:8: R1702: Too many nested blocks (4/3) (too-many-nested-blocks)`
There's quite a few `if` nested blocks in that file and I feel like changing it up will kind of mess up the readability.
Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698536829
@NadimYounes , in case it catches you: please note the file needs to end with an empty line, which you did miss in `tests/providers/ssh/hooks/test_ssh.py` earlier. Not sure if you have fixed it. Just reminding and hope 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] NadimYounes commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
NadimYounes commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698001000
> This change looks good and feels helpful to other people. Could you please add unit tests?
@mik-laj I added the unit tests following the format of the previous ones. Again, I am no expert with unit testing/contributing to larger repositories so please feel free to criticize/offer feedback on my tests. I ran the tests with `pytest` following the airflow guidelines and they all passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG edited a comment on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG edited a comment on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698532008
Hi @NadimYounes . I think the static check is justing putting "too" strict rule on the depth of nested blocks.
I don't favour putting any "ignore" annotation, so the quick fix I can come out with would be to "revoke" the suggestion I have given earlier (https://github.com/apache/airflow/pull/11097#discussion_r493246703). Instead, you go back for your earlier implementation, i.e.
```python
if private_key and not private_key_passphrase:
...
elif private_key and private_key_passphrase:
...
```
In such a way, the depth would be 3 instead of 4. Then it should be ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-697040381
This change looks good and feels helpful to other people. Could you please add unit tests?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG edited a comment on pull request #11097: Added support for encrypted private keys in SSHHook
Posted by GitBox <gi...@apache.org>.
XD-DENG edited a comment on pull request #11097:
URL: https://github.com/apache/airflow/pull/11097#issuecomment-698532008
Hi @NadimYounes . I think the static check is justing putting "too" strict rule on the depth of nested blocks.
I don't favour putting any "ignore" annotation, so the quick fix I can come out with would be to "revoke" the suggestion I have given earlier (https://github.com/apache/airflow/pull/11097#discussion_r493246703). Instead, you go back for your earlier implementation, i.e.
```python
if private_key and not private_key_passphrase:
...
elif private_key and private_key_passphrase:
...
```
In such a way, the depth would be 3 instead of 4. Then it should be ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org