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

Remove v3 api form SSA #98

Merged
merged 1 commit into from Aug 8, 2019

Conversation

borod108
Copy link

@borod108 borod108 commented Jul 28, 2019

Remove the usage of the ovirt gem and version 3 api from the SSA

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

Depends on: ManageIQ/manageiq-providers-ovirt#394

@borod108
Copy link
Author

@roliveri is this ok?

Remove the usage of the ovirt gem and version 3 api from the SSA

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1535782
@miq-bot
Copy link
Member

miq-bot commented Jul 28, 2019

Checked commit borod108@b0b5c73 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

lib/MiqVm/MiqVm.rb

@borod108
Copy link
Author

@miq-bot add_label bug
@miq-bot add_label ivanchuk/yes

@miq-bot miq-bot added the bug label Jul 29, 2019
@miq-bot
Copy link
Member

miq-bot commented Jul 29, 2019

@borod108 Cannot apply the following label because they are not recognized: ivanchuk/yes

@agrare
Copy link
Member

agrare commented Jul 29, 2019

@miq-bot assign @roliveri

@jerryk55
Copy link
Member

I'm not sure this was looked at with regard to other interactions inside the SSA gem. For instance, rhevmVm.disks was removed but it is used in the VmConfig code.

@roliveri
Copy link
Member

@borod108,
as per Jerry's comment, are there still issues with the VmConfig code?

@borod108
Copy link
Author

@jerryk55 can you suggest how I will find all those places? Currently with this code smartstate analysis works, how should I find all the places to change?

@agrare
Copy link
Member

agrare commented Aug 2, 2019

@borod108 could you maintain the same interface but use the v3 api instead? That should fix the issue of having to find every caller

@borod108
Copy link
Author

borod108 commented Aug 2, 2019

@agrare I could, we talked about it with Martin - it just does not seem to be a good solution. Also - if we fix it properly then the knowledge of how this thing actually works will spread to us as well and we will be able to fix things in the future and not keep putting bandages on this.

@borod108
Copy link
Author

borod108 commented Aug 6, 2019

@agrare @roliveri @jerryk55 so how should we proceed?
Can you point me to what else I should change?
I mean it runs on my env and there are no tests for this part that fail so I am not sure what to do here...

@roliveri
Copy link
Member

roliveri commented Aug 7, 2019

@jerryk55 Can you provide a pointer to where it's used in the VmConfig code?

@jerryk55
Copy link
Member

jerryk55 commented Aug 7, 2019

Yeah let me look it up again. Hopefully I'm not mistaken. Or maybe its better if I am. More to come....

@jerryk55
Copy link
Member

jerryk55 commented Aug 8, 2019

I believe I was mistaken. There is a reference to rhevmVm.disks in VMconfig - not rhevmVm.attributes[:disks] - I misread the code. So I remove my objection to this.

@roliveri roliveri merged commit e9cbf92 into ManageIQ:master Aug 8, 2019
@roliveri
Copy link
Member

roliveri commented Aug 8, 2019

@borod108 Will release a new version of the manageiq-smartstate GEM and let you know the version.

@roliveri
Copy link
Member

roliveri commented Aug 8, 2019

@borod108 released version 0.3.0 of the manageiq-smartstate GEM.

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

5 participants