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

Fix graph refresh fetching custom attributes #211

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Fix graph refresh fetching custom attributes #211

merged 1 commit into from
Mar 1, 2018

Conversation

borod108
Copy link
Contributor

@borod108 borod108 commented Feb 20, 2018 •
edited
Loading

Graph refresh fixed to fetch the custom attributes for both full and targeted refresh.

Fix: https://bugzilla.redhat.com/show_bug.cgi?id=1546792

@borod108 borod108 added the wip label Feb 20, 2018
@miq-bot miq-bot removed the wip label Feb 20, 2018
@borod108 borod108 force-pushed the cust_attrs branch 4 times, most recently from 463331b to c4025c3 Compare February 20, 2018 12:07
pkliczewski
pkliczewski reviewed Feb 21, 2018
View reviewed changes
app/models/manageiq/providers/redhat/inventory_collection_default/infra_manager.rb Outdated
@@ -131,6 +131,23 @@ def root_folders(extra_attributes = {})
attributes.merge!(extra_attributes)
end

def custom_attributes(extra_attributes = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to update here instead.

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 am not sure what else the core part is used for, I will ask @Ladas + its faster to merge.

...cr_cassettes/manageiq/providers/redhat/infra_manager/refresh/ovirt_sdk_refresh_recording.yml Outdated
@@ -5253,6 +5253,12 @@ https://localhost:8443/ovirt-engine/api/vms/3a9401a0-bf3d-4496-8acf-edd3e903511f
</topology>
</cpu>
<cpu_shares>0</cpu_shares>
<custom_properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align it with existing content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, actually it does not work for some reason, I will probably delete this part.

...cr_cassettes/manageiq/providers/redhat/infra_manager/refresh/ovirt_sdk_refresh_recording.yml Outdated
@@ -5380,6 +5386,13 @@ https://localhost:8443/ovirt-engine/api/vms/3a9401a0-bf3d-4496-8acf-edd3e903511f
</topology>
</cpu>
<cpu_shares>0</cpu_shares>
<custom_properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

pkliczewski
pkliczewski reviewed Feb 22, 2018
View reviewed changes
app/models/manageiq/providers/redhat/inventory_collection_default/infra_manager.rb
@@ -131,6 +131,23 @@ def root_folders(extra_attributes = {})
attributes.merge!(extra_attributes)
end

def vm_and_template_ems_custom_fields(extra_attributes = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

@agrare do you think we should update here or move this change to core repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's ok to update it here, so the backport is easier

then for the upstream, we should push this to core and see if it aligns correctly with other Infra managers

@borod108 borod108 changed the title wip Fix graph refresh fetching custom attributes Feb 22, 2018
@borod108 borod108 force-pushed the cust_attrs branch 4 times, most recently from 2e7a14f to af09e2a Compare February 25, 2018 08:05
@borod108
Copy link
Contributor Author

Investigate bug Ladas found with targeted template, add to BZ if real

@borod108 borod108 force-pushed the cust_attrs branch 14 times, most recently from 7c5c4d1 to ba8742c Compare February 28, 2018 13:40
@borod108 borod108 force-pushed the cust_attrs branch 2 times, most recently from 183aa18 to 1c7ffc7 Compare February 28, 2018 15:49
Fix graph refresh fetching custom attributes
bcb84da 
Graph refresh fixed to fetch the custom attributes for both full and targeted refresh.

Fix: https://bugzilla.redhat.com/show_bug.cgi?id=1546792
@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2018

Some comments on commit borod108@bcb84da

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_custom_attributes_spec.rb

  • ⚠️ - 10 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 26 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 27 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2018

Checked commit borod108@bcb84da with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 2 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

app/models/manageiq/providers/redhat/infra_manager.rb

@borod108
Copy link
Contributor Author

@masayag please review

@borod108
Copy link
Contributor Author

@Ladas please review

Ladas
Ladas approved these changes Mar 1, 2018
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

looks great 👍

@masayag masayag merged commit 8f9f225 into ManageIQ:master Mar 1, 2018
@mwperina
Copy link

@simaishi Satoe, could you please backport to 5.9?

@masayag
Copy link
Contributor

masayag commented Mar 22, 2018

@miq-bot add_label bug

@mwperina mwperina added the bug label Mar 22, 2018
simaishi pushed a commit that referenced this pull request Mar 22, 2018
@masayag @simaishi
Merge pull request #211 from borod108/cust_attrs
95115c1 
Fix graph refresh fetching custom attributes
(cherry picked from commit 8f9f225)

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

Gaprindashvili backport details:

$ git log -1
commit 95115c13cf152a038af1ae366ef31ac1db112530
Author: Moti Asayag <masayag@redhat.com>
Date:   Thu Mar 1 12:52:57 2018 +0200

    Merge pull request #211 from borod108/cust_attrs
    
    Fix graph refresh fetching custom attributes
    (cherry picked from commit 8f9f2252f7881bf577b29efda25334526c6f70a4)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1559624

@borod108 borod108 deleted the cust_attrs branch December 4, 2018 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkliczewski pkliczewski pkliczewski left review comments

@Ladas Ladas Ladas approved these changes

Assignees
No one assigned
Labels
bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@borod108 @miq-bot @mwperina @masayag @simaishi @pkliczewski @Ladas