[GitHub] [airflow] qgallet commented on pull request #20164: Making SFTPHook's constructor consistent with its superclass SSHHook

[GitBox <gi...@apache.org>]

qgallet commented on pull request #20164:
URL: https://github.com/apache/airflow/pull/20164#issuecomment-1007548480


   I believe this introduces a regression on any use of the SFTPHook that doesn't set a specific sftp_conn_id. Since the parameter has a default value, this is then used to override ssh_conn_id with it.
   
   This happens for instance on the [SFTPToGCSOperator](https://github.com/apache/airflow/blob/f35ad27080a2e1f29efc20a9bd0613af0f6ff2ec/airflow/providers/google/cloud/transfers/sftp_to_gcs.py#L123). It worked on version 2.3.0 of the provider, but now on 2.4.0, no matter what connection is passed in its constructor, the hook will use "sftp_default".
   
   One simple fix would be to remove the default value on sftp_conn_id but I'm not sure it's completely safe and backwards compatible. Or maybe switch the order of the two parameters so that sftp_conn_id is first. What do 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