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.

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

Rewrite ftp file/directory existence and upload mechanisms to address a 550 error #1603

Merged
merged 6 commits into from
Feb 9, 2015

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Feb 6, 2015

The creation of intermediate directories was removed in commit 32df530 as some ftp systems
will allow you to create a directory without the intermediate directories. Some ftp servers, including iis, don't support doing this and raise a 550 error on the first missing directory.

This pull request is more conservative and assumes you must create each intermediate directory one at a time.

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

In a follow up pull request, I'll be adding camcorder tests to test the net/ftp interactions as the specs don't cover end to end testing.

The logging within FileDepotFtp was slightly changed.

Below is an example snippet when the ftp server doesn't have some of the intermediate directories:


[----] I, [2015-02-06T16:03:43.710741 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#with_connection) Connecting through FileDepotFtp: [test ftp]
[----] I, [2015-02-06T16:03:43.710859 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#connect) Connecting to FileDepotFtp: test ftp host: test-ftp.foo.bar.com...
[----] I, [2015-02-06T16:03:43.833582 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#connect) Connected to FileDepotFtp: test ftp host: test-ftp.foo.bar.com
[----] I, [2015-02-06T16:03:44.407360 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#create_directory_structure) creating tmp/user1/default_1000000000001
[----] I, [2015-02-06T16:03:44.541827 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#create_directory_structure) creating tmp/user1/default_1000000000001/EVM_1000000000001
[----] I, [2015-02-06T16:03:44.564096 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#upload) Uploading file: tmp/user1/default_1000000000001/EVM_1000000000001/Current_region_1_default_1000000000001_EVM_1000000000001_20150113_213629_20150206_160343.zip to File Depot: test ftp...
[----] I, [2015-02-06T16:03:50.652019 #51742:3fd4dd465be4]  INFO -- : MIQ(FileDepotFtp#upload) Uploading file: tmp/user1/default_1000000000001/EVM_1000000000001/Current_region_1_default_1000000000001_EVM_1000000000001_20150113_213629_20150206_160343.zip... Complete

@jrafanie
Copy link
Member Author

jrafanie commented Feb 6, 2015

cc @brandondunne @dajohnso

super
with_connection do |ftp|
with_connection do |_ftp|
Copy link
Member

Choose a reason for hiding this comment

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

If you don't need it, then can you just remove the |_ftp| entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@Fryguy Fryguy added the bug label Feb 6, 2015
@bdunne
Copy link
Member

bdunne commented Feb 6, 2015

@jrafanie Looks great! 🍪

The creation of intermediate directories was removed in commit 32df530 as some ftp systems
will allow you to create a directory without the intermediate directories.

This commit is more conservative and assumes you must create each intermediate directory one at a time.

file_exists? now encapsulates issuing ftp 'NLST' with exception handling on missing file/directories.

create_directory_structure will now traverse the desired directory path and create missing intermediate directories.

Move the actual uploading into a private method.
net/ftp methods don't support Pathnames so convert them to Strings.

https://bugzilla.redhat.com/show_bug.cgi?id=1184465
destination_file_exists? was renamed to file_exists? since it's used for the destination directories in addition to the destination file.
destination_path needs to return a Pathname object that responds to join.

https://bugzilla.redhat.com/show_bug.cgi?id=1184465
@jrafanie
Copy link
Member Author

jrafanie commented Feb 6, 2015

@Fryguy I rebased your ideas into the major commit:

diff --git a/vmdb/app/models/file_depot_ftp.rb b/vmdb/app/models/file_depot_ftp.rb
index 7827e3b..5559a01 100644
--- a/vmdb/app/models/file_depot_ftp.rb
+++ b/vmdb/app/models/file_depot_ftp.rb
@@ -18,7 +18,7 @@ class FileDepotFtp < FileDepot

   def upload_file(file)
     super
-    with_connection do |_ftp|
+    with_connection do
       begin
         return if file_exists?(destination_file)

@@ -121,9 +121,8 @@ class FileDepotFtp < FileDepot

   def base_path
     # uri: "ftp://ftp.example.com/incoming" => #<Pathname:incoming>
-    path = URI.split(URI.encode(uri))[5]
-    path = Pathname.new(path)
-    path.relative_path_from(Pathname.new("/"))
+    path = URI(URI.encode(uri)).path
+    Pathname.new(path)
   end

@miq-bot
Copy link
Member

miq-bot commented Feb 6, 2015

Checked commits jrafanie@13ca633 .. jrafanie@a49ae38 with rubocop 0.27.1
3 files checked, 1 offense detected

vmdb/app/models/file_depot_ftp.rb

Fryguy added a commit that referenced this pull request Feb 9, 2015
Rewrite ftp file/directory existence and upload mechanisms to address a 550 error
@Fryguy Fryguy merged commit c685538 into ManageIQ:master Feb 9, 2015
@Fryguy Fryguy deleted the ftp_550_error branch February 9, 2015 14:21
@Fryguy Fryguy added this to the Sprint 19 Ending Feb 16, 2015 milestone Feb 9, 2015
@jrafanie
Copy link
Member Author

jrafanie commented Feb 9, 2015

Added upstream ruby PR to enhance Net::FTP methods such as nlst to accept Pathname objects: ruby/ruby#828

@Fryguy
Copy link
Member

Fryguy commented Feb 9, 2015

Nice @jrafanie 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants