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

Replaces missing Inventory::Builder in refresh runner #117

Merged
merged 1 commit into from
Sep 26, 2018

Conversation

slemrmartin
Copy link
Contributor

@slemrmartin slemrmartin commented Sep 24, 2018

Full refresh fails in ManageIQ::Providers::Kubevirt::InfraManager::RefreshWorker::Runner.

ManageIQ::Providers::Inventory.build should be used instead of ManageIQ::Providers::Inventory::Kubevirt::Builder.build_full, which was removed.


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

caused by PR: #115


Steps to reproduce:

Click on Refresh on Kubevirt provider

Inventory.build is used instead Inventory::Builder.build_full

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1632131
@miq-bot miq-bot added the wip label Sep 24, 2018
@slemrmartin
Copy link
Contributor Author

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Sep 24, 2018
@miq-bot
Copy link
Member

miq-bot commented Sep 24, 2018

Checked commit slemrmartin@a021065 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

parser, collector = ManageIQ::Providers::Kubevirt::Builder.build_full(manager, persister)

# execute parse and persist:
parser.parse
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we run it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inventory.new calls parser.parse

Copy link
Contributor

@pkliczewski pkliczewski left a comment

Choose a reason for hiding this comment

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

Looks ok, once you remove wip I will ack. Please verify it works.

@slemrmartin slemrmartin changed the title [WIP] Replaces missing Inventory::Builder in refresh runner Replaces missing Inventory::Builder in refresh runner Sep 26, 2018
@slemrmartin
Copy link
Contributor Author

@pkliczewski just verified, it's working.

@miq-bot miq-bot removed the wip label Sep 26, 2018
Copy link
Contributor

@pkliczewski pkliczewski left a comment

Choose a reason for hiding this comment

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

Thank you!

@pkliczewski pkliczewski merged commit 30a0ba1 into ManageIQ:master Sep 26, 2018
@slemrmartin slemrmartin deleted the missing-inventory-builder branch September 26, 2018 11:02
@pkliczewski
Copy link
Contributor

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Oct 1, 2018
Replaces missing Inventory::Builder in refresh runner

(cherry picked from commit 30a0ba1)

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

simaishi commented Oct 1, 2018

Hammer backport details:

$ git log -1
commit cc7b388d597244dda513363c5f5427db160c40fa
Author: Piotr Kliczewski <piotr.kliczewski@gmail.com>
Date:   Wed Sep 26 11:18:10 2018 +0200

    Merge pull request #117 from slemrmartin/missing-inventory-builder
    
    Replaces missing Inventory::Builder in refresh runner
    
    (cherry picked from commit 30a0ba19e0403b57a384adb0e19760e9a6136b78)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1632131

pkliczewski added a commit to pkliczewski/manageiq-providers-kubevirt that referenced this pull request Dec 7, 2018
With recent change [1] we introduced this regression where we stopped calling
parser. This PR fixes the issue.

Fixes: https://bugzilla.redhat.com/1649629

[1] ManageIQ#117
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.

4 participants