-
Notifications
You must be signed in to change notification settings - Fork 165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small changes and suggestions, thanks for this PR @JaryN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reacted on review comments.
I detected some fixture changes in commit 637b1eb The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
Please check PRT result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this PR @JaryN. There are a few test failures in PRT, but I think these can be fixed in later PRs, as the majority of tests are now passing.
@@ -40,6 +40,8 @@ class SCVMMProvider(InfraProvider): | |||
settings_key = 'ems_scvmm' | |||
ui_prov_type = 'Microsoft System Center VMM' | |||
log_name = 'scvmm' | |||
provisioning_dialog_widget_names = (InfraProvider.provisioning_dialog_widget_names | |||
- {'environment'} | {'customize'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious but how is this different from - {'environment', 'customize'}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I was like WTF have I done here, but it is different. So perhaps I should rephrase this bit a bit to make it more clear for the reader.
The problem is the operator priority. The result of
(SOME_SET - {'environment'} | {'customize'})
does always include the 'customize' while result of
(SOME_SET - {'environment', 'customize'})
never does.
Should I change this to
((SOME_SET - {'environment'}) | {'customize'})
pattern?
Or rather
SOME_SET.difference({'environment'}).union('{customize'})
or
newset = set(SOME_SET)
newset -= {'environment'}
newset |= '{customize'}
What do you guys find more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I'd say SOME_SET.difference({'environment'}).union('{customize'})
is the most clear, and is consistent with your use of union
and difference
in the other providers.
That said, the logic is sound, and the clarity is subjective, not going to block the PR on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mshriver Ok. I will try keeping this in mind and use this form.
We got some bugs in CFME related to nil deference on provisioning request dialog.
This PR is adding checks to existing tests. These checks are doing clicking trough the request tabs to verify they do display fine.
{pytest: cfme/tests/test_foo_file.py -v cfme/tests/cloud_infra_common/test_cloud_init_provisioning.py cfme/tests/infrastructure/test_provisioning_dialog.py cfme/tests/infrastructure/test_vm_clone.py}
Local runs and test failures advocacy
cfme/tests/cloud_infra_common/test_cloud_init_provisioning.py::test_provision_cloud_init[openstack-13] PASSED
The cfme/tests/cloud_infra_common/test_cloud_init_provisioning.py/test_provision_cloud_init[ec2] seems to be failing because Fedora image is not present on the EC2. It can be fixed in yamls.