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 oVirt provider code #118

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Remove oVirt provider code #118

merged 1 commit into from
Apr 21, 2017

Conversation

jhernand
Copy link
Contributor

Currently the part of the oVirt provider that performs event monitoring
and inventory collection (using version 3 of the oVirt API and the
'ovirt' gem) lives in this repository, in the
'lib/gems/pending/ovirt_provider' directory. As the oVirt provider has
been extracted to a separate repository, it is convenient to move that
code there. The relevant copy has been added to that repository in this
pull request:

Move code from manageiq-gems-pending
ManageIQ/manageiq-providers-ovirt#9

This patch removes the code from this repository.

Note that part of the code can't be moved, or rather, isn't convinient
to move it now. The reason is that code, the 'rhevmTest.rb' file, for
example, doesn't conceptually belong into the oVirt provider. To keep
this code in this repository it is necessary to add a dependency, so
that 'manageiq-gems-pending' will now depend on
'manageiq-providers-ovirt'.

ManageIQ/manageiq-providers-ovirt#8

Currently the part of the oVirt provider that performs event monitoring
and inventory collection (using version 3 of the oVirt API and the
'ovirt' gem) lives in this repository, in the
'lib/gems/pending/ovirt_provider' directory. As the oVirt provider has
been extracted to a separate repository, it is convenient to move that
code there. The relevant copy has been added to that repository in this
pull request:

  Move code from manageiq-gems-pending
  ManageIQ/manageiq-providers-ovirt#9

This patch removes the code from this repository.

Note that part of the code can't be moved, or rather, isn't convinient
to move it now. The reason is that code, the 'rhevmTest.rb' file, for
example, doesn't conceptually belong into the oVirt provider. To keep
this code in this repository it is necessary to add a dependency, so
that 'manageiq-gems-pending' will now depend on
'manageiq-providers-ovirt'.

ManageIQ/manageiq-providers-ovirt#8

Signed-off-by: Juan Hernandez <[email protected]>
@jhernand
Copy link
Contributor Author

@durandom @Fryguy please review.

Note that I am not certain about what to do with the rhevmTest.rb file, and in general with the oVirt-related files in lib/gems/pending/MiqVm/test, as I don't understand what is the purpose of those files. Are they to be considered part of the oVirt provider? Should they be moved to the new repository? If so, how can I test them to make sure they work after moving?

Adding the oVirt provider as a dependency of manageiq-gems-pending is really ugly, but I think it is the correct thing if these files are to be kept in this repository.

@jhernand
Copy link
Contributor Author

jhernand commented Apr 11, 2017

Note that this shouldn't be merged before ManageIQ/manageiq-providers-ovirt#9.

@durandom
Copy link
Member

Note that I am not certain about what to do with the rhevmTest.rb file, and in general with the oVirt-related files in lib/gems/pending/MiqVm/test, as I don't understand what is the purpose of those files

@bdunne Are those tests that are actually run? I'm also not sure what to do with them.
I guess the cleanest way would be to move all rhv related stuff over to the ovirt repo

@bdunne bdunne changed the title Remove oVirt provider code [WIP] Remove oVirt provider code Apr 12, 2017
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Marked as WIP to prevent merge before ManageIQ/manageiq-providers-ovirt#9

@bdunne
Copy link
Member

bdunne commented Apr 12, 2017

@durandom I wouldn't move that. It's related to fleecing code and needs to adapt to pluggable providers. Although, updating the requires and any references to constants would be nice.

@@ -7,6 +7,10 @@ gemspec
gem "handsoap", "~>0.2.5", :require => false, :git => "https://github.com/ManageIQ/handsoap.git", :tag => "v0.2.5-5"
gem "rubywbem", :require => false, :git => "https://github.com/ManageIQ/rubywbem.git", :branch => "rubywbem_0_1_0"

# This is needed because some of the smart analysis tests scripts, insde the 'lib/gems/pending/MiqVm/test' directory,
# use the oVirt provider inventory classes.
gem "manageiq-providers-ovirt", :require => false, :git => "https://github.com/ManageIQ/manageiq-providers-ovirt.git", :branch => "master"
Copy link
Member

Choose a reason for hiding this comment

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

Once this is merged, we'll need to open GH issue in manageiq-releasen repo to update the branch name when we cut gaprindashvili.

@@ -51,7 +51,6 @@ Gem::Specification.new do |s|
s.add_runtime_dependency "nokogiri", "~>1.6.8"
s.add_runtime_dependency "openscap", "~>0.4.3"
s.add_runtime_dependency "ovirt", "~>0.15.1"
s.add_runtime_dependency "parallel", "~>1.9" # For OvirtInventory
Copy link
Member

Choose a reason for hiding this comment

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

@bdunne Even though this is being removed and the comment says "For OvirtInventory", can you please verify this is needed by anyone else that was getting it automatically? azure comes to mind, and also the core repo, but I'm not sure what else.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM 👍 @bdunne Please coordinatethe merges.

@bdunne
Copy link
Member

bdunne commented Apr 21, 2017

@jhernand With ManageIQ/manageiq-providers-ovirt#9 merged, are you ready to remove the [WIP]?

From what I see, the only other references to parallel is marked as TODO. It isn't actually required anywhere.

@bdunne
Copy link
Member

bdunne commented Apr 21, 2017

:) I marked as WIP, unmarking and merging.

@bdunne bdunne changed the title [WIP] Remove oVirt provider code Remove oVirt provider code Apr 21, 2017
@bdunne bdunne removed the wip label Apr 21, 2017
@bdunne bdunne merged commit fe36aef into ManageIQ:master Apr 21, 2017
@bdunne bdunne added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 21, 2017
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.

4 participants