Depreciate private_key_pass extra param and rename to private_key_passphrase #14028

pgillet · 2021-02-02T13:49:56Z

Some operators perform SFTP operations by the means of SFTPHook. SFTPHook delegates the actual SFTP connection to
the pysftp module.

When connecting with a private key protected by a passphrase, the private_key_pass extra parameter must be
specified in the connection, and this corresponds to the arguments' naming in pysftp.

However, SFTPHook inherits from SSHHook which already manages a private_key_passphrase extra param. But this
param is unused in favor of the private_key_pass param introduced in SFTPHook.

For consistency, I propose to drop private_key_pass in SFTPHook and to reuse private_key_passphrase from SSHHook.
This way, a single connection can be used for both SSH and SFTP operations.

boring-cyborg · 2021-02-02T13:50:04Z

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 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 Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
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

RosterIn · 2021-02-03T22:22:48Z

UPDATING.md

+### SFTP connection `private_key_pass` extra param is renamed to `private_key_passphrase`
+
+Some operators perform SFTP operations by the means of `SFTPHook`. `SFTPHook` delegates the actual SFTP connection to
+ the `pysftp` module.
+
+When connecting with a private key protected by a passphrase, the `private_key_pass` extra parameter had to be
+ specified in the connection, and this corresponds to the arguments' naming in `pysftp`.
+
+However, `SFTPHook` inherits from `SSHHook` which already manages a `private_key_passphrase` extra param. But this
+ param was unused in favor of the `private_key_pass` param introduced in `SFTPHook`.
+
+For consistency, `private_key_pass` has been dropped in `SFTPHook` and `private_key_passphrase` is well reused.
+This way, a single connection can be used for both SSH and SFTP operations.
+


I don't think this is the place for this.
It's information about the backward incompatibility of a provider.
If I upgrade a provider I need to see this in the provider release notes not in Airflow release notes.

Correct. Those should be moved to CHANGELOG.rst (and ADDITIONAL_INFO.md for backports till the end of March) in the provider's folders. Those are newly added (just merged some changes recently and once this PR is merged (#14013) the SFTP provider will have both files that should be modified instead. I am releasing new wave of providers today. so this change wil only male it to the next one (but I think it is not urgent and can be released easily in 2-3 weeks when we release the next wave).

✔️ Done

Hi,
I don't know if it's my job to resolve the conversation !?

github-actions · 2021-02-04T12:14:16Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

airflow/providers/ssh/hooks/ssh.py

ashb · 2021-03-08T15:27:46Z

airflow/providers/sftp/hooks/sftp.py

-                if 'private_key_pass' in extra_options:
-                    self.private_key_pass = extra_options.get('private_key_pass')


This makes this PR a breaking change -- could you please add a back-compatiblity path in (i.e. something that notices when this option is set but the new one is not, uses it and issues a deprecation warning)?

✔️ Done

ashb · 2021-03-08T15:29:21Z

airflow/providers/ssh/hooks/ssh.py

@@ -131,9 +132,11 @@ def __init__(  # pylint: disable=too-many-statements
                    self.key_file = extra_options.get("key_file")

                private_key = extra_options.get('private_key')
-                private_key_passphrase = extra_options.get('private_key_passphrase')
+                self.private_key_passphrase = extra_options.get('private_key_passphrase')


Making this an instance variable has no effect -- changing it won't do anything as the value has already been used by the time anything else could set this.

The idea was to reuse it in SFTPHook subclass, not to change it. The private key passphrase is consumed by Paramiko in SSHhook, while it is consumed by Pysftp in SFTPHook. Anyway, I do not inherit this field anymore.

hi @pgillet @ashb will this PR be released in the next provider batch?

…sphrase

github-actions · 2021-06-10T16:51:27Z

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.

boring-cyborg · 2021-06-10T17:20:57Z

Awesome work, congrats on your first merged pull request!

RosterIn reviewed Feb 3, 2021

View reviewed changes

pgillet force-pushed the ssh-sftp-params-consistency branch 3 times, most recently from ce08dcf to 1c7884a Compare February 4, 2021 09:27

pgillet force-pushed the ssh-sftp-params-consistency branch from 1c7884a to 1252bd2 Compare February 4, 2021 13:06

pgillet force-pushed the ssh-sftp-params-consistency branch from 1252bd2 to 67b8fad Compare February 15, 2021 18:11

RosterIn reviewed Feb 28, 2021

View reviewed changes

airflow/providers/ssh/hooks/ssh.py Outdated Show resolved Hide resolved

pgillet force-pushed the ssh-sftp-params-consistency branch from 67b8fad to c842977 Compare March 1, 2021 17:41

ashb requested changes Mar 8, 2021

View reviewed changes

pgillet force-pushed the ssh-sftp-params-consistency branch from c842977 to 198cd1e Compare March 17, 2021 17:43

pgillet changed the title ~~Reuse private_key_passphrase from SSHHook super class~~ Depreciate private_key_pass extra param and rename to private_key_passphrase Mar 17, 2021

pgillet force-pushed the ssh-sftp-params-consistency branch from 198cd1e to 849da71 Compare March 18, 2021 18:00

Depreciate private_key_pass extra param and rename to private_key_pas…

70b9402

…sphrase

pgillet force-pushed the ssh-sftp-params-consistency branch from 849da71 to 70b9402 Compare March 22, 2021 09:23

eladkal added the area:providers label Apr 3, 2021

ashb approved these changes Jun 10, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jun 10, 2021

ashb merged commit 9351e2a into apache:main Jun 10, 2021

potiuk mentioned this pull request Jun 15, 2021

Status of testing Providers that were prepared on June 18, 2021 #16456

Closed

73 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Depreciate private_key_pass extra param and rename to private_key_passphrase #14028

Depreciate private_key_pass extra param and rename to private_key_passphrase #14028

pgillet commented Feb 2, 2021 •

edited

boring-cyborg bot commented Feb 2, 2021

RosterIn Feb 3, 2021

potiuk Feb 3, 2021 •

edited

pgillet Feb 4, 2021

pgillet Feb 8, 2021

github-actions bot commented Feb 4, 2021

ashb Mar 8, 2021

pgillet Mar 17, 2021

ashb Mar 8, 2021

pgillet Mar 17, 2021 •

edited

ashb Mar 17, 2021

alexInhert May 20, 2021

github-actions bot commented Jun 10, 2021

boring-cyborg bot commented Jun 10, 2021

		if 'private_key_pass' in extra_options:
		self.private_key_pass = extra_options.get('private_key_pass')

Depreciate private_key_pass extra param and rename to private_key_passphrase #14028

Depreciate private_key_pass extra param and rename to private_key_passphrase #14028

Conversation

pgillet commented Feb 2, 2021 • edited

boring-cyborg bot commented Feb 2, 2021

Choose a reason for hiding this comment

potiuk Feb 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgillet Mar 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 10, 2021

boring-cyborg bot commented Jun 10, 2021

pgillet commented Feb 2, 2021 •

edited

potiuk Feb 3, 2021 •

edited

pgillet Mar 17, 2021 •

edited