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

FileDepotFtp: FTP.nlst cannot distinguish empty from non-existent dir #9127

Merged
merged 2 commits into from
Jun 15, 2017
Merged

FileDepotFtp: FTP.nlst cannot distinguish empty from non-existent dir #9127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f40f36a
AnonFTP: Create directory structure by descending down
isimluk Jun 3, 2016
f87471a
Start mocking specific behavior of remote ftp servers
isimluk Jun 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions app/models/file_depot_ftp.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,15 @@ def file_exists?(file_or_directory)
private

def create_directory_structure(directory_path)
Pathname.new(directory_path).descend do |path|
next if file_exists?(path)

_log.info("creating #{path}")
ftp.mkdir(path.to_s)
pwd = ftp.pwd
Copy link
Member

Choose a reason for hiding this comment

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

@isimluk I had explicit reasons for using descend.

See #1603

If this works on iis, or any of the many varieties of ftp servers, I'm ok with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. The new code drills down the directory structure by mkdir A; chdir A the same way. So the IIS's inability to create directories like mkdir -p should not be a problem. However, I will try to get some IIS for testing.

Copy link
Member

Choose a reason for hiding this comment

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

@isimluk Please ask around and let me know if you can't get access for testing. Also, if you have ideas on how to capture this behavior so we can automate tests to avoid regression, I'd love to hear any ideas.

I'm fine with the change but would want some sanity checks on various ftps. Maybe @jcarter12 can DM you some information to test against the ftp they use for anonymous access.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if you have ideas on how to capture this behavior so we can automate tests
to avoid regression, I'd love to hear any ideas.

I think the approach I had taken in the second commit of this PR can really help us in that regard.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. I'd like an examples for iis and anonymous ftp that @jcarter12 before merge though. We've changed things for other ftps in the past and I feel like this might cause a regression without confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me too. Will advise QE on testing in the BZ.

directory_path.to_s.split('/').each do |directory|
unless ftp.nlst.include?(directory)
_log.info("creating #{directory}")
ftp.mkdir(directory)
end
ftp.chdir(directory)
end
ftp.chdir(pwd)
end

def upload(source, destination)
Expand Down
76 changes: 67 additions & 9 deletions spec/models/file_depot_ftp_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,28 @@
end

context "#upload_file" do
it "does not already exist" do
expect(file_depot_ftp).to receive(:connect).and_return(connection)
expect(file_depot_ftp).to receive(:file_exists?).exactly(4).times.and_return(false)
expect(connection).to receive(:mkdir).with("uploads")
expect(connection).to receive(:mkdir).with("uploads/#{@zone.name}_#{@zone.id}")
expect(connection).to receive(:mkdir).with("uploads/#{@zone.name}_#{@zone.id}/#{@miq_server.name}_#{@miq_server.id}")
expect(connection).to receive(:putbinaryfile)
expect(log_file).to receive(:post_upload_tasks)
expect(connection).to receive(:close)
it 'uploads file to vsftpd with existing directory structure' do
vsftpd = VsftpdMock.new('uploads' =>
{"#{@zone.name}_#{@zone.id}" =>
{"#{@miq_server.name}_#{@miq_server.id}" => {}}})
expect(file_depot_ftp).to receive(:connect).and_return(vsftpd)
file_depot_ftp.upload_file(log_file)
expect(vsftpd.content).to eq('uploads' =>
{"#{@zone.name}_#{@zone.id}" =>
{"#{@miq_server.name}_#{@miq_server.id}" =>
{"Current_region_unknown_#{@zone.name}_#{@zone.id}_#{@miq_server.name}_#{@miq_server.id}_unknown_unknown.txt" =>
log_file.local_file}}})
end

it 'uploads file to vsftpd with empty /uploads directory' do
vsftpd = VsftpdMock.new('uploads' => {})
expect(file_depot_ftp).to receive(:connect).and_return(vsftpd)
file_depot_ftp.upload_file(log_file)
expect(vsftpd.content).to eq('uploads' =>
{"#{@zone.name}_#{@zone.id}" =>
{"#{@miq_server.name}_#{@miq_server.id}" =>
{"Current_region_unknown_#{@zone.name}_#{@zone.id}_#{@miq_server.name}_#{@miq_server.id}_unknown_unknown.txt" =>
log_file.local_file}}})
end

it "already exists" do
Expand All @@ -53,4 +64,51 @@
file_depot_ftp.upload_file(log_file)
end
end

class FtpMock
attr_reader :pwd, :content
def initialize(content = {})
@pwd = '/'
@content = content
end

def chdir(dir)
newpath = (Pathname.new(pwd) + dir).to_s
if local(newpath).kind_of? Hash
@pwd = newpath
end
end

private

def local(path)
local = @content
path.split('/').each do |dir|
next if dir.empty?
local = local[dir]
raise Net::FTPPermError, '550 Failed to change directory.' if local.nil?
end
local
end
end

class VsftpdMock < FtpMock
def nlst(path = '')
l = local(pwd + path)
l.respond_to?(:keys) ? l.keys : []
rescue
return []
end

def mkdir(dir)
l = local(pwd)
l[dir] = {}
end

def putbinaryfile(local_path, remote_path)
dir, base = Pathname.new(remote_path).split
l = local(dir.to_s)
l[base.to_s] = local_path
end
end
end