Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FTPHook doesn't not allow to change port #39308

Closed
1 of 2 tasks
arollet-decathlon opened this issue Apr 29, 2024 · 7 comments · Fixed by #39465
Closed
1 of 2 tasks

FTPHook doesn't not allow to change port #39308

arollet-decathlon opened this issue Apr 29, 2024 · 7 comments · Fixed by #39465
Assignees
@VelmiraPetkova
Labels
area:providers good first issue kind:bug This is a clearly a bug provider:ftp

Comments

@arollet-decathlon
Copy link

arollet-decathlon commented Apr 29, 2024 •
edited

Apache Airflow Provider(s)

ftp

Versions of Apache Airflow Providers

3.8.0

Apache Airflow version

2.6.3

Operating System

Ubuntu 20.04

Deployment

Official Apache Airflow Helm Chart

Deployment details

Irrelevant as it is in source code

What happened

When setting up an FTP connection with a custom port (different from 21), the FTP Hook does not read it at all and simply passes the hostname and uses the default port set by FTP_PORT in ftplib.FTP_PORT (used by ftplib.FTP.port), thus preventing any modification of default port 21.
PS: The 'connect' function of the FTP class reads port from the self.port attribute;

What you think should happen instead

The FTP Hook should be able to read the port (if set) and pass it to the FTP object created

How to reproduce

Set up any FTP connection with and test it on Airflow UI.

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct
@arollet-decathlon arollet-decathlon added area:providers kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Apr 29, 2024
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Apr 29, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@Lee-W Lee-W added provider:ftp good first issue and removed needs-triage label for new issues that we didn't triage yet labels Apr 30, 2024
@josix
Copy link
Contributor

josix commented Apr 30, 2024 •
edited

@Lee-W I could investigate it first, please feel free to assign it to me.

@Lee-W
Copy link
Member

Lee-W commented Apr 30, 2024

Sure! Just assigned to you.

@VelmiraPetkova
Copy link
Contributor

Hello, I am Velmira and I am a new member. I think I've fixed this bug already. I just need to run tests for the change. If you're still not making progress, I can create a pull request and solve the problem.

@josix
Copy link
Contributor

josix commented May 7, 2024

Hi @VelmiraPetkova sure, please go for it

@Lee-W
Copy link
Member

Lee-W commented May 7, 2024

Thanks, @VelmiraPetkova, for offering the help, and @josix, for the quick confirmation! I'll assign this to @VelmiraPetkova instead

@Lee-W Lee-W assigned VelmiraPetkova and unassigned josix May 7, 2024
@Bowrna
Copy link
Contributor

Bowrna commented May 7, 2024 •
edited

I have this issue fixed in my local setup too. If you need any help, I can guide you @VelmiraPetkova

@VelmiraPetkova VelmiraPetkova mentioned this issue May 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@VelmiraPetkova VelmiraPetkova

Labels
area:providers good first issue kind:bug This is a clearly a bug provider:ftp
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow FTPHook to change port number VelmiraPetkova/airflow
5 participants
@Lee-W @Bowrna @josix @VelmiraPetkova @arollet-decathlon