Skip to content

Comments

refactor(ftp): restructure FtpBackend and introduce FtpCore#5949

Merged
Xuanwo merged 3 commits intoapache:mainfrom
uruemu:split-ftp-core
Apr 6, 2025
Merged

refactor(ftp): restructure FtpBackend and introduce FtpCore#5949
Xuanwo merged 3 commits intoapache:mainfrom
uruemu:split-ftp-core

Conversation

@uruemu
Copy link
Contributor

@uruemu uruemu commented Apr 3, 2025

Which issue does this PR close?

Addresses #5702: Split service logic to backend and core modules

Rationale for this change

What changes are included in this PR?

This pull request refactors the FtpBackend by introducing a new FtpCore struct to hosting connection functionality and state.

Are there any user-facing changes?

@uruemu uruemu marked this pull request as ready for review April 3, 2025 01:42
@uruemu uruemu requested a review from Xuanwo as a code owner April 3, 2025 01:42
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. core services/ftp labels Apr 3, 2025
@uruemu uruemu force-pushed the split-ftp-core branch 2 times, most recently from 989f2e1 to df03438 Compare April 3, 2025 12:28
Copy link
Member

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, Uruemu! Love this.

I have one comment on how the core establishes connections.

password: String,
enable_secure: bool,
pool: OnceCell<bb8::Pool<Manager>>,
core: Arc<FtpCore>,
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Thank you very much. This looks good to me!

@uruemu
Copy link
Contributor Author

uruemu commented Apr 5, 2025

Thanks @erickguan for the review and suggestions. Is this good to merge right away after approval or are there any other prerequisites?

@erickguan
Copy link
Member

@uruemu The last thing is to fix errors from CI.

For example, running clippy: cargo clippy --fix --all-targets --features=services-ftp -- -D warnings.
The other issues are at src/services/ftp/delete.rs:39:40 where this reference to FtpBackend should be FtpCore now.

@uruemu
Copy link
Contributor Author

uruemu commented Apr 5, 2025

@erickguan Fixed the errors. Ready to merge

@Xuanwo
Copy link
Member

Xuanwo commented Apr 6, 2025

Thank you @uruemu for the work and @erickguan for the review. Let's move!

@Xuanwo Xuanwo merged commit 289c8ca into apache:main Apr 6, 2025
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core services/ftp size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants