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

Drop support for oVirt /api, always use /ovirt-engine/api #14469

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Drop support for oVirt /api, always use /ovirt-engine/api #14469

merged 1 commit into from
Apr 6, 2017

Conversation

jhernand
Copy link
Contributor

Versions of oVirt 3.4 and older only supported the /api path for access to the
API. Since version 3.5 /ovirt-engine/api is supported as well, and /api is
deprecated. In version 4.0 /api was removed completely. The provider has
supported both paths, which caused confusion and problems during upgrades. As
we no longer need to support oVirt 3.4 or older, that support can now be
removed. That is what this patch does. The path will be still saved to the
'path' column of the 'endpoints' table, but not used at all.

https://bugzilla.redhat.com/1427653

@jhernand
Copy link
Contributor Author

@borod108 @masayag @pkliczewski @durandom please review.

@jhernand
Copy link
Contributor Author

Note that the relevant changes are only in the api_integration.rb file. The rest is needed to change the hundreds of places where /api needs to be replaced by /ovirt-engine/api in the VC .yml files.

@jhernand
Copy link
Contributor Author

@miq-bot add-labels bug providers/rhevm

@miq-bot
Copy link
Member

miq-bot commented Mar 22, 2017

@jhernand Cannot apply the following label because they are not recognized: bug providers/rhevm

@jhernand
Copy link
Contributor Author

Addressed the Rubcop issues and the offending string issues.

@jhernand
Copy link
Contributor Author

@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2017

This pull request is not mergeable. Please rebase and repush.

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.

code wise LGTM
@oourfali is this fine/yes ?
And I guess you have full oversight of the supported version implications, because reading the attached BZ lost me somehow :)

@@ -10,6 +10,8 @@ module ManageIQ::Providers::Redhat::InfraManager::ApiIntegration
process_api_features_support
end

DEFAULT_PATH = '/ovirt-engine/api'.freeze
Copy link
Member

Choose a reason for hiding this comment

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

is this used somewhere else than below? If not, I would just inline it (put the string where it's used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't used anywhere else. I will inline it.

@jhernand
Copy link
Contributor Author

jhernand commented Apr 4, 2017

Removed the DEFAULT_PATH constant.

@jhernand
Copy link
Contributor Author

jhernand commented Apr 4, 2017

Note that this should be fine/yes and euwe/yes, in my opinion.

Versions of oVirt 3.4 and older only supported the /api path for access to the
API. Since version 3.5 /ovirt-engine/api is supported as well, and /api is
deprecated. In version 4.0 /api was removed completely. The provider has
supported both paths, which caused confusion and problems during upgrades. As
we no longer need to support oVirt 3.4 or older, that support can now be
removed. That is what this patch does. The path will be still saved to the
'path' column of the 'endpoints' table, but not used at all.

https://bugzilla.redhat.com/1427653
@miq-bot
Copy link
Member

miq-bot commented Apr 4, 2017

Checked commit https://github.com/jhernand/manageiq/commit/543846bf888a5acc0eec7595dd517e5458f11727 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@oourfali
Copy link

oourfali commented Apr 4, 2017

LGTM.
And yes @durandom , we are aware as for the supported versions.

@durandom
Copy link
Member

durandom commented Apr 4, 2017

@miq-bot assign @blomquisg
@miq-bot add_label fine/yes, euwe/yes

@blomquisg please merge

@miq-bot miq-bot assigned blomquisg and unassigned oourfali Apr 4, 2017
@blomquisg blomquisg merged commit ac8b311 into ManageIQ:master Apr 6, 2017
@blomquisg blomquisg added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 6, 2017
simaishi pushed a commit that referenced this pull request Apr 6, 2017
…rt_paths

Drop support for oVirt /api, always use /ovirt-engine/api
(cherry picked from commit ac8b311)
@simaishi
Copy link
Contributor

simaishi commented Apr 6, 2017

Fine backport details:

$ git log -1
commit 919c52497b7845a902a525ac14d9525a234ee212
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Thu Apr 6 16:17:14 2017 -0400

    Merge pull request #14469 from jhernand/drop_support_for_multiple_ovirt_paths
    
    Drop support for oVirt /api, always use /ovirt-engine/api
    (cherry picked from commit ac8b311ff4ca26b6258f8962302784135fef1032)

@simaishi
Copy link
Contributor

@jhernand It seems this PR depends on #14159 (which isn't currently marked as euwe/yes). Also, there are a few conflicts.. Can you please create an Euwe PR?

@jhernand
Copy link
Contributor Author

Yes @simaishi , I am preparing the backport.

@jhernand
Copy link
Contributor Author

@simaishi the euwe backport is here:

[EUWE] Drop support for oVirt /api, always use /ovirt-engine/api
#14786

@simaishi
Copy link
Contributor

Backported to Euwe via #14786

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.

9 participants