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

ovirt inventory add creation_time #34

Merged
merged 4 commits into from
Apr 29, 2020

Conversation

mnecas
Copy link
Member

@mnecas mnecas commented Apr 28, 2020

add creation_time to ovirt inventory

@@ -127,6 +128,8 @@ def _get_dict_of_struct(self, vm):
'fqdn': vm.fqdn,
'os_type': vm.os.type,
'template': self.connection.follow_link(vm.template).name,
'creation_time': str(vm.creation_time),
'creation_time_timestamp': vm.creation_tim.timestamp() if vm.creation_tim.tzinfo else vm.creation_tim.replace(tzinfo=timezone.utc).timestamp(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Following would work:

vm.creation_time.strftime('%s') # For Python2
vm.creation_time.timestamp() # For python3

And my Ansible Tower is python2 default as its RHEL 7 based. For RHEL8+Py3 based .timestamp() would work. Its now up to you if you want to try supporting Py2 which is dead. I guess its my team who need to update RHEL8 if we want to make this work for us.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do NOT want to care for Py2, then update your PR as follows please:

            'creation_time': str(vm.creation_time),
            'creation_time_timestamp': vm.creation_time.timestamp()

That's all, nothing else, you can remove if/else and importing timezone. Thanks for opening this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This script is created for python3 #!/usr/bin/python3 and we distribute the collection rpm to el8 and fc30.
I'll remove the if statement and leave it as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

This script is created for python3 #!/usr/bin/python3 and we distribute the collection rpm to el8 and fc30.

The shebang has no effect as I understand it, because this is not called in a subprocess like the old scripts were. It is imported directly into the main Ansible process in the inventory loading stage.

You can confirm this by running in a python2 virtual environment and applying:

diff --git a/plugins/inventory/ovirt.py b/plugins/inventory/ovirt.py
index 3a8736f..73c88a4 100755
--- a/plugins/inventory/ovirt.py
+++ b/plugins/inventory/ovirt.py
@@ -106,6 +106,8 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
              :return dict of vm struct type
         '''
 
+        import sys
+        raise Exception('version: ' + str(sys.version))
         vms_service = self.connection.system_service().vms_service()
         clusters_service = self.connection.system_service().clusters_service()
         vm_service = vms_service.vm_service(vm.id)

If you're invoking Ansible itself with python2, this runs with python2.

Dropping support for python2 isn't crazy, of course, but if you do, you should put it in the requirements for the plugin, because it's not implied.

@mnecas mnecas force-pushed the ovirt_inventory_add_creation_time branch from 1daa0bb to 08bf6c6 Compare April 29, 2020 05:29
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

+1

@mnecas mnecas merged commit f86f497 into oVirt:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants