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

VMWare - Rename _facts to _info #57474

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@pratikgadiya12
Copy link
Contributor

commented Jun 6, 2019

Fixes: #57278

SUMMARY

Renames VMWare _facts to _info since they do not return ansible_facts.
Continues #56822
Fixes #57278

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

vcenter_extension_facts
vmware_about_facts
vmware_category_facts
vmware_cluster_facts
vmware_datastore_facts
vmware_drs_group_facts
vmware_drs_rule_facts
vmware_dvs_portgroup_facts
vmware_guest_boot_facts
vmware_guest_customization_facts
vmware_guest_disk_facts
vmware_guest_facts
vmware_guest_snapshot_facts
vmware_host_capability_facts
vmware_host_config_facts
vmware_host_dns_facts
vmware_host_feature_facts
vmware_host_firewall_facts
vmware_host_ntp_facts
vmware_host_package_facts
vmware_host_service_facts
vmware_host_ssl_facts
vmware_host_vmhba_facts
vmware_host_vmnic_facts
vmware_local_role_facts
vmware_local_user_facts
vmware_portgroup_facts
vmware_resource_pool_facts
vmware_tag_facts
vmware_target_canonical_facts
vmware_vm_facts
vmware_vmkernel_facts
vmware_vswitch_facts

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@pratikgadiya12 This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from 2804922 to a2b1769 Jun 6, 2019

@Akasurde Akasurde changed the title VMWare - Rename _facts to _info [WIP] VMWare - Rename _facts to _info Jun 6, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch 2 times, most recently from 32b367b to c98bfd6 Jun 6, 2019

@ansibot ansibot removed the ci_verified label Jun 7, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from c98bfd6 to df8d1cb Jun 7, 2019

@pratikgadiya12 pratikgadiya12 changed the title [WIP] VMWare - Rename _facts to _info VMWare - Rename _facts to _info Jun 7, 2019

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

The patch is open for the review :)
@Akasurde @felixfontein @pmarques @samccann

@felixfontein
Copy link
Contributor

left a comment

Looks good, except the renamed module return values.

Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vcenter_extension_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_about_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_category_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_drs_group_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_drs_rule_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_portgroup_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_resource_pool_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_target_canonical_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_vmkernel_info.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/vmware/vmware_vswitch_info.py Outdated

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from df8d1cb to d903360 Jun 10, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

When you force-push, please do not rebase! That's really a PITA to review.

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from d903360 to c2dce06 Jun 10, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from c2dce06 to f4bcba1 Jun 10, 2019

@felixfontein
Copy link
Contributor

left a comment

I think this is done now. @Akasurde can you take a look?

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Sorry for adding the conflict (merged another rename PR). When you rebase, could you insert the entries for your module before the new entries? I'll soon create another PR and will add entries for that one after them, then our PRs won't conflict :)

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Sorry for adding the conflict (merged another rename PR). When you rebase, could you insert the entries for your module before the new entries? I'll soon create another PR and will add entries for that one after them, then our PRs won't conflict :)

Sure, working on it

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_57278 branch from f4bcba1 to 523b45b Jun 10, 2019

@felixfontein
Copy link
Contributor

left a comment

LGTM

@Akasurde

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@felixfontein @pratikgadiya12 Thanks for taking so much efforts for this PR. We are currently refactoring VMware testcases in major way, so I would like to hold the PR for a while. Once we have that PR in place I will merge this. Thanks for the patients.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@Akasurde I hope this won't introduce a lot of conflicts with this PR, since it also has to change several of the tests.

@pmarques
Copy link
Contributor

left a comment

Looks good, except the renamed module return values.

@felixfontein that will break backwards compatibility. should we return both for now? something like:

module.exit_json(changed=False,
    hosts_vmnics_facts = host_vmnic_mgr.gather_host_vmnic_info(), 
    hosts_vmnics_info = host_vmnic_mgr.gather_host_vmnic_info())

I understand that this will likely slowdown a bit the plays

@@ -282,8 +285,8 @@ def main():
except Exception as exc:
module.fail_json(msg="Fact gather failed with exception %s" % to_text(exc))

This comment has been minimized.

Copy link
@pmarques

pmarques Jun 13, 2019

Contributor

Should we also change "Fact gather failed" to "** Unable to gather information**" as ater the else statement?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 13, 2019

Contributor

Sounds like a good idea. There are probably some more such messages. These can also be updated in subsequent PRs IMO.

@@ -124,9 +125,11 @@ def main():
],
supports_check_mode=True,
)
if module._name == 'vmware_host_ntp_facts':
module.deprecate("The 'vmware_host_ntp_facts' module has been renamed to 'vmware_host_ntp_info'", version='2.13')

vmware_host_ntp_config = VmwareNtpFactManager(module)

This comment has been minimized.

Copy link
@pmarques

pmarques Jun 13, 2019

Contributor

Should we also remove VmwareNtpFactManager to ** VmwareNtpManager**? As well as the class definition above, can't comment there :(

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 13, 2019

Contributor

This is internal to the module and not visible to the outside, so it can also be changed later. I think it's more important to first fix everything visible to the user.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Looks good, except the renamed module return values.

@felixfontein that will break backwards compatibility.

Exactly, that's why this no longer happens in this PR.

should we return both for now? something like:

module.exit_json(changed=False,
    hosts_vmnics_facts = host_vmnic_mgr.gather_host_vmnic_info(), 
    hosts_vmnics_info = host_vmnic_mgr.gather_host_vmnic_info())

I would stick to the old name, until there is a good mechanism to deprecate / rename return values. Currently there isn't.

def gather_vswitch_facts(self):
"""Gather vSwitch facts"""
def gather_vswitch_info(self):
"""Gather vSwitch info"""
hosts_vswitch_facts = dict()

This comment has been minimized.

Copy link
@pmarques

pmarques Jun 13, 2019

Contributor

should we also rename this one? (and everything else in the method)

@@ -13,26 +13,26 @@

# Local user manager works only with standalone ESXi server
- &user_fact_data

This comment has been minimized.

Copy link
@pmarques

pmarques Jun 13, 2019

Contributor

Should we also change the tag? minor thing I know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.