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

New provider event parsing #14399

Merged
merged 1 commit into from
Apr 3, 2017
Merged

New provider event parsing #14399

merged 1 commit into from
Apr 3, 2017

Conversation

borod108
Copy link

Add parsing events with OvirtSDK, this respects the setting use_ovirt_engine_sdk and will use only Ovirt gem if it is set to false.

This PR is base on: #14398

@pkliczewski @masayag please review

pkliczewski
pkliczewski reviewed Mar 20, 2017
View reviewed changes
app/models/manageiq/providers/redhat/infra_manager/ovirt_services.rb Outdated
@@ -0,0 +1,3 @@
module ManageIQ::Providers::Redhat::InfraManager::OvirtServices
require_nested :OvirtServices
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue with this line. We do not have OvirtServices in OvirtServices directory.
We should have here:

require_nested :Builder

Copy link
Author

Choose a reason for hiding this comment

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

right, thank you!

@borod108 borod108 force-pushed the rfe/new_provider_event_parsing branch from b185ded to 6d79889 Compare March 20, 2017 11:02
@borod108 borod108 mentioned this pull request Mar 20, 2017
Closed
@borod108 borod108 force-pushed the rfe/new_provider_event_parsing branch from 6d79889 to 1f55add Compare March 20, 2017 11:42
@borod108 borod108 force-pushed the rfe/new_provider_event_parsing branch 2 times, most recently from 6d79889 to 5f98c41 Compare March 20, 2017 15:42
@Fryguy Fryguy changed the title Rfe/new provider event parsing New provider event parsing Mar 20, 2017
@borod108 borod108 changed the title New provider event parsing [WIP] New provider event parsing Mar 20, 2017
@miq-bot miq-bot added the wip label Mar 20, 2017
@borod108 borod108 force-pushed the rfe/new_provider_event_parsing branch 2 times, most recently from ad527f0 to a05d568 Compare March 29, 2017 10:58
@borod108 borod108 changed the title [WIP] New provider event parsing New provider event parsing Mar 29, 2017
@miq-bot miq-bot removed the wip label Mar 29, 2017
@borod108
Copy link
Author

@durandom Fixed tests and rubocops.
@pkliczewski and @masayag if you want to look at this again before the merge...

@durandom
Copy link
Member

@miq-bot add_label fine/yes

durandom
durandom approved these changes Mar 30, 2017
View reviewed changes
Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

besides some small nitpicks 👍

@borod108 if you want to address the comments, you can also squash the commits into one?

Then give your go, if you want me to ping blomquisg for merging

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb Outdated
@@ -111,6 +111,11 @@ def rhevm_inventory
@rhevm_inventory ||= connect(:service => "Inventory")
end

def ovirt_services
ManageIQ::Providers::Redhat::InfraManager::OvirtServices::Builder.new(self)
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to memoize this?

@ovirt_services ||= begin
  ManageIQ::Providers::Redhat::InfraManager::OvirtServices::Builder.new(self).build.new(:ems => self)
end

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb Outdated
end

def username_by_href(href)
username = nil
Copy link
Member

Choose a reason for hiding this comment

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

username is unnecessary

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb Outdated
end

def username_by_href(href)
username = nil
Copy link
Member

Choose a reason for hiding this comment

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

same here, username is uneccessary

Add event parsing with OvirtSDK
c589205 
Add parsing events with OvirtSDK, this respects the setting use_ovirt_engine_sdk
and will use only Ovirt gem if it is set to false.
@borod108 borod108 force-pushed the rfe/new_provider_event_parsing branch from a05d568 to c589205 Compare April 2, 2017 18:51
@miq-bot
Copy link
Member

miq-bot commented Apr 2, 2017

Some comments on commit borod108@c589205

spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb

  • ⚠️ - 73 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 75 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 76 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Apr 2, 2017

Checked commit borod108@c589205 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 2 offenses detected

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

@borod108
Copy link
Author

borod108 commented Apr 3, 2017

@durandom all changes are done.

@durandom
Copy link
Member

durandom commented Apr 3, 2017

@miq-bot assign @blomquisg

@blomquisg please merge

@miq-bot miq-bot assigned blomquisg and unassigned oourfali Apr 3, 2017
@blomquisg blomquisg requested a review from agrare April 3, 2017 16:49
@blomquisg blomquisg removed the request for review from agrare April 3, 2017 19:40
@blomquisg blomquisg merged commit ebcb31c into ManageIQ:master Apr 3, 2017
@blomquisg blomquisg added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 3, 2017
simaishi pushed a commit that referenced this pull request Apr 3, 2017
@blomquisg @simaishi
Merge pull request #14399 from borod108/rfe/new_provider_event_parsing
54986a4 
New provider event parsing
(cherry picked from commit ebcb31c)
@simaishi
Copy link
Contributor

simaishi commented Apr 3, 2017

Fine backport details:

$ git log -1
commit 54986a4391fa7398a1222ee0e3169e26a185f2cd
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Mon Apr 3 15:40:47 2017 -0400

    Merge pull request #14399 from borod108/rfe/new_provider_event_parsing
    
    New provider event parsing
    (cherry picked from commit ebcb31c965e011cf279dc748f7afbdfd40a476bf)

@durandom durandom mentioned this pull request Apr 10, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkliczewski pkliczewski pkliczewski approved these changes

@durandom durandom durandom approved these changes

Assignees
No one assigned
Labels
enhancement fine/backported providers/infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@borod108 @durandom @miq-bot @simaishi @pkliczewski @blomquisg @Fryguy @chessbyte @oourfali