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

Actually apply CloudManager's api_version #219

Merged
merged 1 commit into from
Apr 3, 2018
Merged

Actually apply CloudManager's api_version #219

merged 1 commit into from
Apr 3, 2018

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Mar 26, 2018 •
edited
Loading

We're only storing API version in VMDB, but we never actually apply it when performing API calls (so it always defaults to 5.1)! It didn't really matter until now, because most of the API was implemented in v1.5. Acquiring WebMKS ticket, however, requires >= 5.5.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1560517

Merge in one take with: ManageIQ/manageiq-ui-classic#3701

@miq-bot add_label enhancement,gaprindashvili/yes
@miq-bot assign @agrare

@agrare
Copy link
Member

agrare commented Mar 28, 2018

@miha-plesko looks good to me just need to address the test failures

@miha-plesko
Copy link
Contributor Author

@agrare done, thanks for pointing out.

agrare
agrare reviewed Mar 29, 2018
View reviewed changes
app/models/manageiq/providers/vmware/manager_auth_mixin.rb
end

module ClassMethods
def raw_connect(server, port, username, password, validate = false)
def raw_connect(server, port, username, password, api_version = '5.5', validate = false)
Copy link
Member

Choose a reason for hiding this comment

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

If you change the args to raw_connect you'll need to update the UI task args here https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/mixins/ems_common_angular.rb#L124

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱 Glad to have such a great reviewer, thank you so much, everything would break. Here's the PR ManageIQ/manageiq-ui-classic#3701, best to merge both at same time.

Copy link
Member

Choose a reason for hiding this comment

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

It was top-of-mind because @slemrmartin is looking at improving the way we validate credentials for exactly this reason 😄

@agrare
Copy link
Member

agrare commented Mar 29, 2018

@miha-plesko looks good, just put a PR in to ui-classic and I'll merge this

This was referenced Mar 29, 2018
Merged
Merged
Merged
@miha-plesko
Actually apply CloudManager's api_version
41c05ae 
We're only storing API version in VMDB, but we never actually apply
it when performing API calls (so it defaults to 5.1)! It didn't really
matter until now, because most of the API was implemented in v1.5.
Acquiring WebMKS ticket, however, requires >5.5.

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
@miha-plesko
Copy link
Contributor Author

I pushed a slight change on this one to explicitly pick API version 5.5 in rspec. To make API version more obvious and gem-independent. Especially because for some reason refresh_parser modifies EMS' api_version which makes relying on default value very dangerous...

@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2018

Checked commit miha-plesko@41c05ae with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare agrare merged commit 41c05ae into ManageIQ:master Apr 3, 2018
agrare added a commit that referenced this pull request Apr 3, 2018
@agrare
Merge pull request #219 from miha-plesko/actually-apply-api-version
961b39a 
Actually apply CloudManager's api_version
@agrare agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018
simaishi pushed a commit that referenced this pull request Apr 3, 2018
@agrare @simaishi
Merge pull request #219 from miha-plesko/actually-apply-api-version
83aa379 
Actually apply CloudManager's api_version
(cherry picked from commit 961b39a)

https://bugzilla.redhat.com/show_bug.cgi?id=1563364
@simaishi
Copy link
Contributor

simaishi commented Apr 3, 2018

Gaprindashvili backport details:

$ git log -1
commit 83aa379c73878d93544a1e0c0b5647458cb3c100
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Apr 3 08:41:35 2018 -0400

    Merge pull request #219 from miha-plesko/actually-apply-api-version
    
    Actually apply CloudManager's api_version
    (cherry picked from commit 961b39a1ebbe875c9a281a3873508b03a1942119)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1563364

@miha-plesko miha-plesko deleted the actually-apply-api-version branch January 7, 2019 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miha-plesko @agrare @miq-bot @simaishi