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

Renderer fails when calling validate_import_vm #3

Merged
merged 1 commit into from Apr 11, 2017

Conversation

pkliczewski
Copy link
Contributor

When using automation we see that there is an issue in validate_import_vm
and there is a time when api_version is not avialbale. We need to make sure
that validation passes.

@pkliczewski
Copy link
Contributor Author

This is moved PR [1] from the main repo

ManageIQ/manageiq#14610

@pkliczewski
Copy link
Contributor Author

@miq-bot assign @durandom

@pkliczewski
Copy link
Contributor Author

@agrare @borod108 please take a look

@@ -31,7 +31,8 @@ def import_vm(source_vm_id, target_params)
end

def validate_import_vm
api_version >= '4.0'
version = highest_supported_api_version
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can use: (highest_supported_api_version is almost entirely memorized, if the caching fails it will return nil and not run twice anyway)
highest_supported_api_version >= '4' if highest_supported_api_version

Copy link
Contributor

Choose a reason for hiding this comment

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

or actually:
highest_supported_api_version && highest_supported_api_version >= '4'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

When using automation we see that there is an issue in validate_import_vm
and there is a time when api_version is not avialbale. We need to make sure
that validation passes.
@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2017

Checked commit pkliczewski@a048839 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,7 +31,7 @@ def import_vm(source_vm_id, target_params)
end

def validate_import_vm
api_version >= '4.0'
highest_supported_api_version && highest_supported_api_version >= '4'
Copy link
Member

Choose a reason for hiding this comment

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

I find >= on Strings dangerous and strange.
Also highest_supported_api_version seems to return 3 (int) as a default....
Consider some of those conversions:

[3] pry(main)> 3 >= '4'
ArgumentError: comparison of Fixnum with String failed
from (pry):3:in `>='
[4] pry(main)> 3.to_i >= 4
=> false
[5] pry(main)> '3.0'.to_i
=> 3
[6] pry(main)> Integer('3.0')
ArgumentError: invalid value for Integer(): "3.0"
from (pry):6:in `Integer'
[7] pry(main)> '3.0beta'.to_i
=> 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked the code and we seems to return a string

2.3.0 :001 > ems = ExtManagementSystem.last
PostgreSQLAdapter#log_after_checkout, connection_pool: size: 5, connections: 1, in use: 1, waiting_in_queue: 0
ExtManagementSystem Load (0.4ms) SELECT "ext_management_systems".* FROM "ext_management_systems" ORDER BY "ext_management_systems"."id" DESC LIMIT $1 [["LIMIT", 1]]
ExtManagementSystem Inst Including Associations (6430.5ms - 1rows)
=> #<ManageIQ::Providers::Redhat::InfraManager id: 11, name: "ovirt", created_on: "2017-04-11 14:18:50", updated_on: "2017-04-11 14:19:15", guid: "c89c81a0-1ec1-11e7-b1c4-c85b76722276", zone_id: 1, type: "ManageIQ::Providers::Redhat::InfraManager", api_version: "4.2.0_master.", uid_ems: nil, host_default_vnc_port_start: nil, host_default_vnc_port_end: nil, provider_region: nil, last_refresh_error: nil, last_refresh_date: "2017-04-11 14:19:15", provider_id: nil, realm: nil, tenant_id: 1, project: nil, parent_ems_id: nil, subscription: nil, last_metrics_error: nil, last_metrics_update_date: nil, last_metrics_success_date: nil, tenant_mapping_enabled: nil>
2.3.0 :002 > ems.highest_supported_api_version
Authentication Load (0.7ms) SELECT "authentications".* FROM "authentications" WHERE "authentications"."resource_id" = $1 AND "authentications"."resource_type" = $2 [["resource_id", 11], ["resource_type", "ExtManagementSystem"]]
Authentication Inst Including Associations (105.1ms - 1rows)
Endpoint Load (0.3ms) SELECT "endpoints".* FROM "endpoints" WHERE "endpoints"."resource_id" = $1 AND "endpoints"."resource_type" = $2 [["resource_id", 11], ["resource_type", "ExtManagementSystem"]]
Endpoint Inst Including Associations (16.4ms - 2rows)
=> "4"

I understand that it looks strange but this is how the code works.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ok I just wonder why this returns Integer(3) ...

Copy link
Contributor

Choose a reason for hiding this comment

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

@durandom good point, will change.

Copy link
Contributor

Choose a reason for hiding this comment

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

here: #10

@durandom durandom added the bug label Apr 11, 2017
@durandom durandom added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 11, 2017
@durandom durandom merged commit ac28230 into ManageIQ:master Apr 11, 2017
@simaishi simaishi added fine/yes and removed fine/no labels Apr 28, 2017
@simaishi
Copy link
Contributor

@simaishi
Copy link
Contributor

Fine backport (to manageiq repo) details:

$ git log -1
commit 0a5744dc1aa3e9e67bffe208b9c204460d1e1c44
Author: Marcel Hild <hild@b4mad.net>
Date:   Tue Apr 11 16:43:23 2017 +0200

    Merge pull request #3 from pkliczewski/master
    
    Renderer fails when calling validate_import_vm
    (cherry picked from commit ac2823005455374be8c511655a0d514b561fde8b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1446613

agrare pushed a commit to agrare/manageiq-providers-ovirt that referenced this pull request May 16, 2017
borod108 pushed a commit that referenced this pull request Jul 15, 2019
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.

None yet

6 participants