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

Fix .encode('hex') call for python3 #53343

Open
wants to merge 10 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@carchi8py
Copy link
Contributor

carchi8py commented Mar 5, 2019

SUMMARY

Python 3 no longer supports .encode('hex'), this results in this module crashing in python 3. This bug fix

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • na_ontap_lun_map.py
ADDITIONAL INFORMATION

return_value = {
'lun_node': lun.get_child_content('node'),
'lun_ostype': lun.get_child_content('multiprotocol-type'),
'lun_serial': lun.get_child_content('serial-number'),
'lun_naa_id': '600a0980' + lun.get_child_content('serial-number').encode('hex'),
'lun_naa_id': '600a0980' + str(naa_hex),

This comment has been minimized.

@bcoca

bcoca Mar 5, 2019

Member

see to_text functions in ansible.module_utils._text, they are py2/3 compatible

This comment has been minimized.

@lonico

lonico Mar 13, 2019

Contributor

We tried to use to_text, but the feedback I got from our engineer is that codecs was more appropriate here as we are dealing with hexadecimal.

This comment has been minimized.

@gundalow

gundalow Mar 19, 2019

Contributor

AFAIK the Ansible functions should work, could you please provide a sample input value and expected output value given that input.

This comment has been minimized.

@lonico

lonico Mar 20, 2019

Contributor

Maybe we misunderstood. We thought to_text would remove the need to import codecs and use hexlify.
We now believed you meant to use to_text rather than str.encode and decode. Trying this.

@angystardust

This comment has been minimized.

Copy link
Contributor

angystardust commented Mar 5, 2019

Confirmed to work on Fedora29 with python 3.7 👍

ansible 2.7.7
  config file = /home/user/.ansible.cfg
  configured module search path = ['/home/user/Code/ansible/library']
  ansible python module location = /usr/lib/python3.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.7.2 (default, Jan 16 2019, 19:49:22) [GCC 8.2.1 20181215 (Red Hat 8.2.1-6)]
@ansibot

This comment has been minimized.

@lonico

lonico approved these changes Mar 13, 2019

Copy link
Contributor

lonico left a comment

shipit

@ansibot ansibot added shipit and removed core_review labels Mar 13, 2019

@JohnLieske
Copy link

JohnLieske left a comment

Screenshot from 2019-03-20 11-54-25

GTG Looks good.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 20, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 5 errors:

lib/ansible/modules/storage/netapp/na_ontap_lun_map.py:0:0: E319 RETURN.lun_naa_id.type: not a valid value for dictionary value @ data['lun_naa_id']['type']. Got 'string'
lib/ansible/modules/storage/netapp/na_ontap_lun_map.py:0:0: E319 RETURN.lun_node.type: not a valid value for dictionary value @ data['lun_node']['type']. Got 'string'
lib/ansible/modules/storage/netapp/na_ontap_lun_map.py:0:0: E319 RETURN.lun_ostype.type: not a valid value for dictionary value @ data['lun_ostype']['type']. Got 'string'
lib/ansible/modules/storage/netapp/na_ontap_lun_map.py:0:0: E319 RETURN.lun_serial.type: not a valid value for dictionary value @ data['lun_serial']['type']. Got 'string'
lib/ansible/modules/storage/netapp/na_ontap_lun_map.py:0:0: E319 RETURN.lun_state.type: not a valid value for dictionary value @ data['lun_state']['type']. Got 'string'

click here for bot help

@lonico

lonico approved these changes Mar 20, 2019

@lonico

This comment has been minimized.

Copy link
Contributor

lonico commented Mar 20, 2019

shipit

@ansibot ansibot added shipit and removed core_review labels Mar 20, 2019

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.