From a4e1447635f9b1275de7879210fd45c16d8d29e6 Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Tue, 26 Feb 2019 17:08:28 -0500 Subject: [PATCH 1/4] default merged_uri should return the parameter not the attribute A previous PR changed merged_uri to return the attribute on the object rather than the passed in parameter. This is incorrect and is changed herein. Only Swift handles this differently. --- app/models/file_depot.rb | 2 +- spec/models/file_depot_ftp_spec.rb | 4 ++-- spec/models/file_depot_nfs_spec.rb | 8 ++------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/models/file_depot.rb b/app/models/file_depot.rb index 67f371fe577..3f0aa2024e5 100644 --- a/app/models/file_depot.rb +++ b/app/models/file_depot.rb @@ -33,7 +33,7 @@ def upload_file(file) @file = file end - def merged_uri(_uri, _api_port) + def merged_uri(uri, _api_port) uri end end diff --git a/spec/models/file_depot_ftp_spec.rb b/spec/models/file_depot_ftp_spec.rb index 6825ab18f9c..1f08aa95d57 100644 --- a/spec/models/file_depot_ftp_spec.rb +++ b/spec/models/file_depot_ftp_spec.rb @@ -122,8 +122,8 @@ def putbinaryfile(local_path, remote_path) file_depot_ftp.uri = uri end - it "should return the uri attribute from the file depot object and ignore the parameter" do - expect(file_depot_ftp.merged_uri(nil, nil)).to eq uri + it "should ignore the uri attribute from the file depot object and return the parameter" do + expect(file_depot_ftp.merged_uri(nil, nil)).to eq nil end end end diff --git a/spec/models/file_depot_nfs_spec.rb b/spec/models/file_depot_nfs_spec.rb index a805a2d3a5e..c2c282366a6 100644 --- a/spec/models/file_depot_nfs_spec.rb +++ b/spec/models/file_depot_nfs_spec.rb @@ -12,12 +12,8 @@ file_depot_nfs.uri = uri end - it "should return the uri set on the depot object and ignore the uri parameter" do - expect(file_depot_nfs.merged_uri(swift_uri, nil)).to eq uri - end - - it "should return the uri set on the depot object and ignore an empty uri parameter" do - expect(file_depot_nfs.merged_uri(nil, nil)).to eq uri + it "should ignore the uri set on the depot object and return the uri parameter" do + expect(file_depot_nfs.merged_uri(swift_uri, nil)).to eq swift_uri end end end From 14682c6b47234371bc87b1e96506554d9afef2de Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 28 Feb 2019 10:49:57 -0500 Subject: [PATCH 2/4] Make URI arguments more meaningful Make the ignored URI attribute and actual parameter more meaningful. --- spec/models/file_depot_nfs_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/file_depot_nfs_spec.rb b/spec/models/file_depot_nfs_spec.rb index c2c282366a6..d8ee4e0060d 100644 --- a/spec/models/file_depot_nfs_spec.rb +++ b/spec/models/file_depot_nfs_spec.rb @@ -1,6 +1,6 @@ describe FileDepotNfs do - let(:uri) { "nfs://foo.com/directory" } - let(:swift_uri) { "swift://foo_bucket/doo_directory" } + let(:ignore_uri) { "nfs://ignore.com/directory" } + let(:actual_uri) { "nfs://actual_bucket/doo_directory" } let(:file_depot_nfs) { FileDepotNfs.new(:uri => uri) } it "should return a valid prefix" do @@ -9,11 +9,11 @@ describe "#merged_uri" do before do - file_depot_nfs.uri = uri + file_depot_nfs.uri = ignore_uri end it "should ignore the uri set on the depot object and return the uri parameter" do - expect(file_depot_nfs.merged_uri(swift_uri, nil)).to eq swift_uri + expect(file_depot_nfs.merged_uri(actual_uri, nil)).to eq actual_uri end end end From 803b1a580481e39143b044f99db3411ae3c18a3a Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Fri, 1 Mar 2019 10:59:49 -0500 Subject: [PATCH 3/4] Add verify_file_depot tests Add tests to validate that MiqSchedule.verify_file_depot does the right thing with regard to calling FileDepot.merged_uri. --- spec/factories/file_depot.rb | 4 ++++ spec/models/miq_schedule_spec.rb | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/spec/factories/file_depot.rb b/spec/factories/file_depot.rb index c3f765acd08..685ee697861 100644 --- a/spec/factories/file_depot.rb +++ b/spec/factories/file_depot.rb @@ -5,7 +5,11 @@ end factory(:file_depot_ftp, :class => "FileDepotFtp", :parent => :file_depot) { uri { "ftp://somehost/export" } } + factory(:file_depot_swift, :class => "FileDepotSwift", :parent => :file_depot) { uri { "swift://swifthost/swiftbucket" } } factory :file_depot_ftp_with_authentication, :parent => :file_depot_ftp do after(:create) { |x| x.authentications << FactoryBot.create(:authentication) } end + factory :file_depot_swift_with_authentication, :parent => :file_depot_swift do + after(:create) { |x| x.authentications << FactoryBot.create(:authentication) } + end end diff --git a/spec/models/miq_schedule_spec.rb b/spec/models/miq_schedule_spec.rb index 42ea322bde5..cf0bbbf46f9 100644 --- a/spec/models/miq_schedule_spec.rb +++ b/spec/models/miq_schedule_spec.rb @@ -690,6 +690,24 @@ expect(FileDepot.count).to eq(1) expect(MiqSchedule.count).to eq(0) end + + let(:swift_file_depot) { FactoryBot.create(:file_depot_swift_with_authentication) } + let(:swift_params) { {:uri_prefix => "swift", :uri => "swift://swift.example.com/test_depot", :name => "Test Backup Swift Depot", :username => "user", :password => "password"} } + let(:swift_run_at) { {:interval => {:value => "1", :unit => "hourly"}, :start_time => "2012-03-10 01:35:00 Z", :tz => "Central Time (US & Canada)"} } + + it "does not merge the swift uri if the port is blank" do + swift_schedule = FactoryBot.create(:miq_schedule_validation, :run_at => swift_run_at, :file_depot => swift_file_depot, :sched_action => {:method => "db_backup"}, :resource_type => "DatabaseBackup") + swift_schedule.verify_file_depot(swift_params.merge(:save => true)) + expect(swift_schedule.file_depot.uri).to eq(swift_params[:uri]) + end + + it "merges the swift uri and port if the port is specified" do + another_swift_schedule = FactoryBot.create(:miq_schedule_validation, :run_at => swift_run_at, :file_depot => swift_file_depot, :sched_action => {:method => "db_backup"}, :resource_type => "DatabaseBackup") + swift_api_port = 5000 + merged_uri = "swift://swift.example.com:5000/test_depot" + another_swift_schedule.verify_file_depot(swift_params.merge(:swift_api_port => swift_api_port, :save => true)) + expect(another_swift_schedule.file_depot.uri).to eq(merged_uri) + end end describe ".updated_since" do From f1e0d1691b83b8d07436907b956c908edd66914f Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Mon, 4 Mar 2019 17:56:10 -0500 Subject: [PATCH 4/4] Fix NFS File Depot Spec Test Tried to rename the "uri" attribute inadvertantly in previous incarnation of this test and broke it. Fixing now. --- spec/models/file_depot_nfs_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/file_depot_nfs_spec.rb b/spec/models/file_depot_nfs_spec.rb index d8ee4e0060d..d3f1db93d65 100644 --- a/spec/models/file_depot_nfs_spec.rb +++ b/spec/models/file_depot_nfs_spec.rb @@ -1,5 +1,5 @@ describe FileDepotNfs do - let(:ignore_uri) { "nfs://ignore.com/directory" } + let(:uri) { "nfs://ignore.com/directory" } let(:actual_uri) { "nfs://actual_bucket/doo_directory" } let(:file_depot_nfs) { FileDepotNfs.new(:uri => uri) } @@ -9,7 +9,7 @@ describe "#merged_uri" do before do - file_depot_nfs.uri = ignore_uri + file_depot_nfs.uri = uri end it "should ignore the uri set on the depot object and return the uri parameter" do