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

Update parse_new_target to standard refresh hashes #13

Merged
merged 6 commits into from
Jun 21, 2017
Merged

Update parse_new_target to standard refresh hashes #13

merged 6 commits into from
Jun 21, 2017

Conversation

agrare
Copy link
Member

@agrare agrare commented Apr 18, 2017

This will allow parse_new_target to work with standard save_ems_inventory

@agrare
Copy link
Member Author

agrare commented Apr 18, 2017

The refresh_new_target from an event in RHV really needs some tests around it
@borod108 @pkliczewski can you get an event that would be passed to https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/models/manageiq/providers/redhat/infra_manager/event_parser.rb#L60 so that I can test refresh_new_target with what the EventParser returns?

@miq-bot miq-bot added the wip label Apr 18, 2017
@agrare agrare mentioned this pull request Apr 18, 2017
Merged
2 tasks
@agrare agrare force-pushed the refactor_refresh_new_target branch from a71cc1f to 37a0499 Compare April 18, 2017 19:44
@pkliczewski
Copy link
Contributor

@agrare I will add a sepc

@pkliczewski
Copy link
Contributor

@agrare please take a look at #14

@agrare agrare force-pushed the refactor_refresh_new_target branch from 37a0499 to cf27946 Compare April 25, 2017 19:39
@agrare agrare force-pushed the refactor_refresh_new_target branch 6 times, most recently from bbca8a6 to b52db3b Compare May 4, 2017 16:16
@agrare
Copy link
Member Author

agrare commented May 4, 2017

Okay I got parse_new_target linking the new VM with the datacenter and cluster, @pkliczewski @masayag can you guys review?

This is needed to return hashes compatible with save_ems_inventory, see ManageIQ/manageiq#14247

@agrare agrare changed the title [WIP] Update parse_new_target to standard refresh hashes Update parse_new_target to standard refresh hashes May 4, 2017
@miq-bot miq-bot removed the wip label May 4, 2017
pkliczewski
pkliczewski reviewed May 8, 2017
View reviewed changes
spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_spec.rb
target_hash, target_klass, target_find = ep_class.parse_new_target(add_vm_event, description, @ems, name)

new_vm = VCR.use_cassette("#{described_class.name.underscore}_target_new_vm", :allow_unused_http_interactions => true) do
EmsRefresh.refresh_new_target(@ems, target_hash, target_klass, target_find)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a failure in this line just an old dependency issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this uses the new arguments, ManageIQ/manageiq#14247 will have to be merged first for this to be green.
You can check out that branch and test, it passes for me locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Let's make sure that the dependency is merged and I am OK with this PR.

@agrare
Copy link
Member Author

agrare commented May 10, 2017

Thanks guys :) I'll kick the tests when ManageIQ/manageiq#14247 goes in

agrare pushed a commit to agrare/manageiq-providers-ovirt that referenced this pull request May 16, 2017
@durandom
Merge pull request ManageIQ#13 from jntullo/virtual_template_update
258dacc 
Required Virtual Template Parameters
@agrare agrare force-pushed the refactor_refresh_new_target branch from b52db3b to 48355ce Compare June 7, 2017 14:01
@Fryguy Fryguy closed this Jun 16, 2017
@Fryguy Fryguy reopened this Jun 16, 2017
@agrare
Copy link
Member Author

agrare commented Jun 16, 2017

cc @pkliczewski @masayag the dependent PR has been merged and CI is green

@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2017

This pull request is not mergeable. Please rebase and repush.

@agrare agrare force-pushed the refactor_refresh_new_target branch from 48355ce to 01612f6 Compare June 20, 2017 15:48
@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2017

Checked commits agrare/manageiq-providers-ovirt@fe60e02~...01612f6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member Author

agrare commented Jun 20, 2017

Rebased due to #36 moving the main body of the event_parser

@agrare
Copy link
Member Author

agrare commented Jun 21, 2017

@miq-bot assign oourfali

@oourfali oourfali merged commit 4d39634 into ManageIQ:master Jun 21, 2017
@agrare agrare deleted the refactor_refresh_new_target branch June 21, 2017 12:29
@borod108 borod108 mentioned this pull request Jul 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masayag masayag masayag approved these changes

@pkliczewski pkliczewski pkliczewski approved these changes

Assignees

@oourfali oourfali

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@agrare @pkliczewski @miq-bot @masayag @Fryguy @oourfali