Making SFTPHook's constructor consistent with its superclass SSHHook

[saveriogzz]

This is just a first dummy solution that probably scores low for elegance since I'm making both the parameters Optional.
What do you guys think? I will adjust according to your suggestions :)

related issue: #20144

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[potiuk]

I think adding a test for that would be nice too.

[potiuk]

(and fixing static checks - I heartily recommend installing pre-commit. It helps with the iterations a LOT.

[github-actions]

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.

[potiuk]

Looks good. I thin what it needs is a bit more docstring description on which parameter is preferred but looks good otherwise.

[potiuk]

You can also take a look at examples and see which connection is used there (and align with the preferred one)

[saveriogzz]

I am writing some tests and I will send a complete PR as soon as I'm done!

You can also take a look at examples and see which connection is used there (and align with the preferred one)

Do you mean example dags?

[potiuk]

Do you mean example dags?

Yep.

[saveriogzz]

I didn't find any example dag including the SftpHook or Operator!

[potiuk]

I didn't find any example dag including the SftpHook or Operator!

😱 How about adding one ?

[potiuk]

Ah.. I think example for that one can be added later :)

[saveriogzz]

😱 How about adding one ?

Will do!

[qgallet]

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. 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 ?

[saveriogzz]

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 should be fixed by #20756

[potiuk]

Yeah. I yanked the 2.4.0 release and 2.4.1rc1 with the fix included is up for voting. We will release it likely on Wednesday

[saveriogzz]

Yeah. I yanked the 2.4.0 release and 2.4.1rc1 with the fix included is up for voting. We will release it likely on Wednesday

Thanks Jarek! Sorry for the mistake in my PR

[potiuk]

Thanks Jarek! Sorry for the mistake in my PR

No worries :) Happens. That's why we have so flexible mechanims and split the release to separate providers that we can easily handle those kind of situations :).