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

[1LP][RFR] De-duplicate code for VM / VMCollection retirement #9839

Merged
merged 1 commit into from
Jan 31, 2020
Merged

[1LP][RFR] De-duplicate code for VM / VMCollection retirement #9839

merged 1 commit into from
Jan 31, 2020

Conversation

tpapaioa
Copy link
Contributor

@tpapaioa tpapaioa commented Jan 13, 2020

This is a follow-up to #9540, making some additional changes that were suggested in review.

1.) cleanup of code that loops over vm instances, e.g.,

-    for num in range(2):
-        vms[num].cleanup_on_provider()
+    for vm in vms:
+        vm.cleanup_on_provider()

2.) De-duplication of the common code shared by the set_retirement_date methods in the VM and VMCollection classes. The parameter validation and form filling is the same in both classes, and the only differences are in:

  • The navigation to the Set/Remove Retirement Date view
  • The view instantiation / validation after saving or cancelling any retirement date changes.

I've created a RetirementMixin class and moved the set_retirement_date method there. The code that needs to be overridden at the VM or VMCollection level is moved to the new navigate_to_set_retirement and post_set_retirement methods.

{{ pytest: cfme/tests/cloud_infra_common/test_retirement.py -v --long-running --use-provider vsphere67-nested --use-provider ec2west -k 'test_retirement_now or test_set_retirement or test_unset_retirement_date or test_resume_retired_instance' }}

@tpapaioa tpapaioa changed the title [WIPTEST] De-duplicate code for VM / VMCollection retirement [RFR] De-duplicate code for VM / VMCollection retirement Jan 14, 2020
@john-dupuy john-dupuy added enhancement test-cleanup Test removal, collection changes, re-organization labels Jan 16, 2020
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.

Some small comment, but overall LGTM, awesome work @tpapaioa

cfme/common/vm.py Outdated Show resolved Hide resolved
cfme/common/vm.py Outdated Show resolved Hide resolved
cfme/common/vm.py Outdated Show resolved Hide resolved
@john-dupuy john-dupuy changed the title [RFR] De-duplicate code for VM / VMCollection retirement [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement Jan 16, 2020
@tpapaioa tpapaioa changed the title [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement [1LP][RFR] De-duplicate code for VM / VMCollection retirement Jan 17, 2020
@tpapaioa
Copy link
Contributor Author

One PRT test in the latest commit failed with a TimedOutError exception, but it's unrelated to this PR, so I'm changing the status back to RFR.

@tpapaioa tpapaioa changed the title [1LP][RFR] De-duplicate code for VM / VMCollection retirement [1LP][WIP] De-duplicate code for VM / VMCollection retirement Jan 27, 2020
@dajoRH dajoRH added the WIP label Jan 27, 2020
@tpapaioa tpapaioa changed the title [1LP][WIP] De-duplicate code for VM / VMCollection retirement [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement Jan 27, 2020
@dajoRH dajoRH added WIP-testing and removed WIP labels Jan 27, 2020
@tpapaioa tpapaioa changed the title [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement [1LP][WIP] De-duplicate code for VM / VMCollection retirement Jan 27, 2020
@dajoRH dajoRH added WIP and removed WIP-testing labels Jan 27, 2020
@tpapaioa tpapaioa changed the title [1LP][WIP] De-duplicate code for VM / VMCollection retirement [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement Jan 28, 2020
@dajoRH dajoRH added WIP-testing and removed WIP labels Jan 28, 2020
@tpapaioa tpapaioa changed the title [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement [1LP][RFR] De-duplicate code for VM / VMCollection retirement Jan 28, 2020
@mshriver mshriver self-assigned this Jan 29, 2020
cfme/common/vm.py Outdated Show resolved Hide resolved
cfme/fixtures/vm.py Outdated Show resolved Hide resolved
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.

Two small things to discuss, otherwise these changes look great, thanks @tpapaioa !

@mshriver mshriver changed the title [1LP][RFR] De-duplicate code for VM / VMCollection retirement [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement Jan 29, 2020
cfme/common/vm.py Outdated Show resolved Hide resolved
cfme/fixtures/vm.py Outdated Show resolved Hide resolved
@dajoRH
Copy link
Contributor

dajoRH commented Jan 29, 2020

I detected some fixture changes in commit cc2e271

The local fixture retire_vm_pair is used in the following files:

  • cfme/tests/cloud_infra_common/test_retirement.py
    • test_retirement_now_multiple
    • test_set_retirement_date_multiple
    • test_set_retirement_offset_multiple

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@tpapaioa tpapaioa changed the title [1LP][WIPTEST] De-duplicate code for VM / VMCollection retirement [1LP][RFR] De-duplicate code for VM / VMCollection retirement Jan 29, 2020
@mshriver mshriver merged commit 57e2acd into ManageIQ:master Jan 31, 2020
@tpapaioa tpapaioa deleted the vm_retirement_fixes branch January 31, 2020 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement lint-ok test-cleanup Test removal, collection changes, re-organization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants