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

Add queue_name to ems operations within CloudVolume::Operations mixin #19608

Merged
merged 4 commits into from
Dec 9, 2019
Merged

Add queue_name to ems operations within CloudVolume::Operations mixin #19608

merged 4 commits into from
Dec 9, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 6, 2019

This PR adds a queue_name to the queue options for the CloudVolume::Operations#attach_volume_queue method. I've also added some specs (this mixin doesn't appear to have had existing test coverage), and added some comments on those methods.

Part of #19543

@djberg96
Copy link
Contributor Author

djberg96 commented Dec 6, 2019

@miq-bot add_reviewer @agrare

@djberg96
Copy link
Contributor Author

djberg96 commented Dec 6, 2019

@agrare I'm not sure how sensical these specs are with me currently re-using the same ems for the method argument. If you have a suggestion here, please let me know.

@miq-bot miq-bot requested a review from agrare December 6, 2019 19:05

context "queued methods" do
it "queues an attach task with attach_volume_queue" do
task_id = cloud_volume.attach_volume_queue(user.userid, ems.id, disk.id)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think ems.id is correct here, it should be a vm's ems_ref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I think I need some clarification for how/where a VM fits into this.

Copy link
Member

Choose a reason for hiding this comment

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

A cloud volume is being attached to a VM/instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare ok, updated.

@@ -0,0 +1,46 @@
RSpec.describe 'CloudVolume::Operations' do
let(:ems) { FactoryBot.create(:ems_vmware) }
let(:vm) { FactoryBot.create(:vm_vmware, :ext_management_system => ems) }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to use a vmware ems or vm, particurally for a cloud_volume spec. There are ems_cloud and vm_cloud factories that can keep things more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 looks good

@miq-bot
Copy link
Member

miq-bot commented Dec 9, 2019

Checked commits https://github.com/djberg96/manageiq/compare/340f9ec3dabeb95a5be526bb360d72e80ce78dc9~...d5b3649a303380e7a6c1813f0d276207604c4674 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@agrare
Copy link
Member

agrare commented Dec 9, 2019

UI and API cross-repo tests were green, core was red for a different reason. Will merge this when green.

agrare added a commit that referenced this pull request Dec 9, 2019
…_name

Add queue_name to ems operations within CloudVolume::Operations mixin
@agrare agrare merged commit d5b3649 into ManageIQ:master Dec 9, 2019
@agrare agrare added this to the Sprint 126 Ending Dec 9, 2019 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants