-
Notifications
You must be signed in to change notification settings - Fork 897
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
Fix amazon cloud volume spec failures #13886
Fix amazon cloud volume spec failures #13886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factory part looks great
cc @Ladas
@@ -1,8 +1,8 @@ | |||
describe CloudVolume do | |||
it ".available" do | |||
disk = FactoryGirl.create(:disk) | |||
cv1 = FactoryGirl.create(:cloud_volume_amazon, :attachments => [disk]) | |||
cv2 = FactoryGirl.create(:cloud_volume_amazon) | |||
cv1 = FactoryGirl.create(:cloud_volume, :attachments => [disk]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for using a base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmcculloug
Can you please review this one
@@ -2,9 +2,6 @@ | |||
factory :cloud_volume do | |||
end | |||
|
|||
factory :cloud_volume_amazon, :class => "ManageIQ::Providers::Amazon::CloudManager::CloudVolume", :parent => :cloud_volume do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to see this go. Looks like we dont use it in the other repos too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good call, I checked the rest of ManageIQ/manageiq
to see if it was used but not the other repos.
You're right though it looks like these are the only cases that use it https://github.com/search?q=org%3AManageIQ+cloud_volume_amazon&type=Code
@agrare |
Checked commits agrare/manageiq@e6740e5~...a98f61f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 spec/models/cloud_volume_spec.rb
|
@mkanoor so it looks like miq_ae_service_manageiq-providers-amazon-cloud_manager-cloud_volume.rb is already replaced by miq_ae_service_manageiq-providers-amazon-storage_manager-ebs-cloud_volume.rb |
@mkanoor do the service model changes make sense? |
cc @blomquisg |
Looks good. May have to deal with model namespace change if users are looking up directly from automate but that is unlikely. @blomquisg Let's merge to get back to 💚 . |
Amazon CloudVolumes are now defined in the
StorageManager
per ManageIQ/manageiq-providers-amazon#136This fixes the associated spec tests failures
cc @durandom @mkanoor