Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[1LP][RFR] Migration Plans Refactoring #8404

Merged
merged 4 commits into from
Apr 5, 2019

Conversation

sshveta
Copy link
Contributor

@sshveta sshveta commented Jan 28, 2019

Migration Plan code refactoring .

juwatts:
{{ pytest: cfme/tests/v2v/test_migration_plan.py --use-provider rhv42 --use-provider vsphere67-nested --provider-limit 2 -vvvv --long-running}}

Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

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

Overall, looking really good! Just had a couple of comments and questions. 👍

cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Show resolved Hide resolved
cfme/v2v/migration_plans.py Show resolved Hide resolved
Copy link
Contributor

@digitronik digitronik left a comment

Choose a reason for hiding this comment

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

Nice PR 👍 I added some comments please have a glance.

cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
@digitronik digitronik changed the title [RFR] Migration Plans Refactoring [WIP] Migration Plans Refactoring Jan 29, 2019
@sshveta sshveta changed the title [WIP] Migration Plans Refactoring [RFR] Migration Plans Refactoring Jan 29, 2019
@sshveta sshveta requested a review from izapolsk January 30, 2019 05:11
Copy link
Contributor

@digitronik digitronik left a comment

Choose a reason for hiding this comment

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

Please add changes. My bad.

cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
cfme/v2v/migration_plans.py Outdated Show resolved Hide resolved
@digitronik digitronik changed the title [RFR] Migration Plans Refactoring [1LP][WIPTEST] Migration Plans Refactoring Jan 30, 2019
@sshveta sshveta changed the title [1LP][WIPTEST] Migration Plans Refactoring [1LP][RFR] Migration Plans Refactoring Jan 30, 2019
@dajoRH dajoRH changed the title [1LP][RFR] Migration Plans Refactoring [1LP][WIP] Migration Plans Refactoring Jan 31, 2019
@ManageIQ ManageIQ deleted a comment from dajoRH Jan 31, 2019
@sshveta sshveta changed the title [1LP][WIP] Migration Plans Refactoring [1LP][RFR] Migration Plans Refactoring Jan 31, 2019
@dajoRH dajoRH changed the title [1LP][RFR] Migration Plans Refactoring [1LP][WIP] Migration Plans Refactoring Jan 31, 2019
@dajoRH dajoRH added WIP and removed WIP labels Jan 31, 2019
@sshveta sshveta changed the title [1LP][WIP] Migration Plans Refactoring [1LP][WIPTEST] Migration Plans Refactoring Jan 31, 2019
@sshveta sshveta changed the title [1LP][WIPTEST] Migration Plans Refactoring [1LP][RFR] Migration Plans Refactoring Jan 31, 2019
@dajoRH dajoRH changed the title [1LP][RFR] Migration Plans Refactoring [1LP][WIP] Migration Plans Refactoring Feb 1, 2019
@dajoRH dajoRH removed the lint-ok label Feb 1, 2019
Copy link
Member

@mshriver mshriver left a comment

Choose a reason for hiding this comment

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

Several things to discuss, but the structure is looking much stronger, with view classes controlling their fill patterns.

@jawatts jawatts changed the title [1LP][RFR] Migration Plans Refactoring [1LP][WIPTEST] Migration Plans Refactoring Apr 3, 2019
"""Overriding fill method of TextInput to send
enter after filling """

def fill(self, value):
Copy link
Contributor Author

@sshveta sshveta Apr 3, 2019

Choose a reason for hiding this comment

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

After_fill is not working , it is not called , hence overriding Fill.

def after_fill(self):
    self.browser.send_keys(Keys.ENTER, self)

@dajoRH dajoRH added needs-lint and removed lint-ok labels Apr 3, 2019
@sshveta sshveta changed the title [1LP][WIPTEST] Migration Plans Refactoring [1LP][RFR] Migration Plans Refactoring Apr 3, 2019
@sshveta sshveta requested a review from mshriver April 4, 2019 00:13
@izapolsk
Copy link
Contributor

izapolsk commented Apr 4, 2019

@sshveta, looks good at first sight. all tests have been skipped in PRT. why ?

@sshveta
Copy link
Contributor Author

sshveta commented Apr 5, 2019

Test passed on PRT

@izapolsk izapolsk merged commit d8a116d into ManageIQ:master Apr 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants