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

Add filtering of image attributes #348

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

andyvesel
Copy link
Contributor

Added filtering in options from unnecessary data, which doesn't belong to image attributes

@andyvesel andyvesel force-pushed the image_attrs_filtering branch from 4e64438 to 52146a7 Compare September 10, 2018 11:59
@andyvesel
Copy link
Contributor Author

@aufi could you review?

@@ -99,6 +99,8 @@ def self.create_image(ext_management_system, create_options)

def raw_update_image(options)
ext_management_system.with_provider_connection(:service => 'Image') do |service|
image_attrs = self.attributes
Copy link
Member

Choose a reason for hiding this comment

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

This seems to fix issue with miq_queue keys in attributes, but to have it fully correct, it is needed check attributes against Fog Image instead of MIQ Template.

self.attributes are attributes of Template class, service.images.find_by_id(ems_ref) returns Fog object - https://github.com/fog/fog-openstack/blob/master/lib/fog/image/openstack/v2/models/image.rb its attributes should be checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR

@alexander-demicev
Copy link

Should this filtering also be added to create action?

@andyvesel andyvesel force-pushed the image_attrs_filtering branch from 52146a7 to 6e7f446 Compare September 20, 2018 12:50
@andyvesel andyvesel force-pushed the image_attrs_filtering branch from 6e7f446 to 1b7fc7f Compare October 2, 2018 14:36
@miq-bot
Copy link
Member

miq-bot commented Oct 2, 2018

Checked commits andyvesel/manageiq-providers-openstack@5583530~...1b7fc7f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/manageiq/providers/openstack/cloud_manager/template.rb

@mansam
Copy link
Contributor

mansam commented Oct 3, 2018

Looks fine, but why are invalid parameters making it into the update call in the first place?

@andyvesel
Copy link
Contributor Author

andyvesel commented Oct 15, 2018

Looks fine, but why are invalid parameters making it into the update call in the first place?

@mansam, For some reason in options hash we getting unneeded parameters like miq_task and miq_task_id. (i.e. {"id" => "37", "name" => "foo", :miq_task => 1800, :miq_task_id => 1200})
Fog Image have no attributes like miq_task and miq_task_id, so I intended to keep in options only those attributs, which are belong to image.
We've no BZ for this. It's a kind of workaround, but with this "filtering" updating of images works correctly

@mansam
Copy link
Contributor

mansam commented Oct 15, 2018

@andyvesel Alright, thanks for the explanation. This workaround is fine, but we should discover why these incorrect parameters are reaching this method.

@andyvesel
Copy link
Contributor Author

@andyvesel Alright, thanks for the explanation. This workaround is fine, but we should discover why these incorrect parameters are reaching this method.

Agree with that, I'll figure this out

@mansam
Copy link
Contributor

mansam commented Oct 16, 2018

@mansam mansam merged commit a5fa4a2 into ManageIQ:master Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants