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

removed incoming as default destination folder for anonymous ftp #6829

Merged
merged 2 commits into from Mar 1, 2016
Merged

removed incoming as default destination folder for anonymous ftp #6829

merged 2 commits into from Mar 1, 2016

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Feb 19, 2016

Fixed several issues with anonymous ftp:

  1. removed 'incoming' as default destination folder
  2. allow creation of subfolders on destination server
  3. supply account name

https://bugzilla.redhat.com/show_bug.cgi?id=1307019

@chessbyte
Copy link
Member

@yrudman I think there needs to be some explanation for the reviewers of why you are deleting all this code and why you think that nothing will break after.

@chessbyte chessbyte added the core label Feb 19, 2016
@jrafanie
Copy link
Member

@yrudman in addition to what @chessbyte says, we should have a rspec test that tests that the destination_path is a specific value... to confim that it no longer has a leading incoming directory

@yrudman
Copy link
Contributor Author

yrudman commented Feb 20, 2016

FTP transfer is fully implemented in FileDepotFtp class.
FileDepotFtpAnonymous < FileDeportFtp.
Removed methods from FileDepotFtpAnonymous were overriding methods from parent class and adding some restrictions:

  • appending “incoming” to destination folder
  • not allowing creation of subfolder on destination server
  • not allowing removing log file
    Those methods belong to class which implements ‘one way limited ftp’ - FileDepotFtpAnonymousRedhatDropbox and they were moved to that class.

@Fryguy
Copy link
Member

Fryguy commented Feb 22, 2016

@bdunne Please review? Does this affect the CloudForms custom Ftp class we have?

@bdunne
Copy link
Member

bdunne commented Feb 22, 2016

@Fryguy Yes, it does. I commented on the downstream code change. Please hold off merging this until that is in place.

@bdunne
Copy link
Member

bdunne commented Feb 24, 2016

@Fryguy Downstream changes merged, this is ready to go

@yrudman yrudman changed the title Removed limitations on anonymous ftp [WIP]Removed limitations on anonymous ftp Feb 26, 2016
@chessbyte chessbyte changed the title [WIP]Removed limitations on anonymous ftp [WIP] Remove limitations on anonymous ftp Feb 26, 2016
@yrudman yrudman force-pushed the removed_incoming_as_default_folder_for_anonymous_ftp branch from d9bd3c4 to c5a5c18 Compare March 1, 2016 19:44
@yrudman yrudman force-pushed the removed_incoming_as_default_folder_for_anonymous_ftp branch from c5a5c18 to 2315c1e Compare March 1, 2016 19:48
@yrudman yrudman changed the title [WIP] Remove limitations on anonymous ftp Remove limitations on anonymous ftp Mar 1, 2016
@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2016

Checked commits yrudman/manageiq@2315c1e~...24a921e with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
55 files checked, 0 offenses detected
Everything looks good. ⭐

@chessbyte chessbyte removed the wip label Mar 1, 2016
@yrudman yrudman changed the title Remove limitations on anonymous ftp removed incoming as default destination folder for anonymous ftp Mar 1, 2016
@jrafanie
Copy link
Member

jrafanie commented Mar 1, 2016

Looks good.

jrafanie added a commit that referenced this pull request Mar 1, 2016
@jrafanie
Merge pull request #6829 from yrudman/removed_incoming_as_default_fol…
5848d73 
…der_for_anonymous_ftp

removed incoming as default destination folder for anonymous ftp
@jrafanie jrafanie merged commit 5848d73 into ManageIQ:master Mar 1, 2016
@jrafanie jrafanie added this to the Sprint 37 Ending Mar 7, 2016 milestone Mar 1, 2016
@bdunne bdunne mentioned this pull request Mar 1, 2016
Merged
@bdunne
Copy link
Member

bdunne commented Mar 1, 2016

@yrudman @jrafanie Were the additional changes re-implemented in downstream code?

bdunne
bdunne reviewed Mar 1, 2016
View reviewed changes
spec/models/file_depot_ftp_anonymous_spec.rb
describe FileDepotFtpAnonymous do
it "should require credentials for anonymous" do
expect(FileDepotFtpAnonymous.requires_credentials?).to eq true
expect(FileDepotFtpAnonymous.new.login_credentials[0]).to eq "anonymous"
Copy link
Member

Choose a reason for hiding this comment

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

In the future, this should probably match the entire array instead of the first element.

@yrudman
Copy link
Contributor Author

yrudman commented Mar 1, 2016

@bdunne I think there is no need in additional change for downstream code. I've tested it with latest upstream code - works

@yrudman yrudman deleted the removed_incoming_as_default_folder_for_anonymous_ftp branch March 2, 2016 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yrudman @chessbyte @jrafanie @Fryguy @bdunne @miq-bot