Skip to content

Conversation

@RachitSharma2001
Copy link
Contributor

The corresponding issue: #26531

I have added the FTPSFileTransmitOperator, which allows for writing to files in FTPS servers and downloading them to the local directory. I have also updated the docs and system tests for this new operator.

I have also added a few more tests in the tests/providers/ftp/operators directory. Because the only difference between the FTPFileTransmitOperator and FTPSFileTransmitOperator is the use of the FTPSHook (as opposed to the FTPHook), I only added unit tests that tested the functionality and use of the FTPSHook within the FTPSFileTransmitOperator.

@RachitSharma2001 RachitSharma2001 force-pushed the ftps_transmit branch 2 times, most recently from 924a6c9 to 008fcd5 Compare December 12, 2022 22:05
@RachitSharma2001 RachitSharma2001 force-pushed the ftps_transmit branch 2 times, most recently from ca524b1 to fdfa6bf Compare December 15, 2022 00:08
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to double check this is intentional and not supposed to be FTPSOperation.PUT

Copy link
Contributor Author

@RachitSharma2001 RachitSharma2001 Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is intentional, as FTPS still uses the same operations (PUT and GET) as FTPFileTransmitOperator. On second thought, I was wondering if that name is misleading. Do you think I should change the name to something like Operation or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be alright.

@ferruzzi
Copy link
Contributor

Left a last question. If that's good to go then I think it looks good.

@RachitSharma2001
Copy link
Contributor Author

Hi everyone, I was wondering if I could get some additional reviews on this PR? No rush, but was just wondering if these changes look good or if there is anything else that should be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants