Skip to content

Commit

Permalink
Added Unicode support for string properties.
Browse files Browse the repository at this point in the history 
Details:

- In Ansible modules implemented in python using AnsibleModule,
  any string input parameters are converted to binary strings,
  before they are made available in module.params. This caused
  comparisons between binary strings and unicode strings in
  the zhmc_partition module, resulting in reporting a change
  when in fact there was no change. This commit converts
  the binary strings to unicode, for some input properties
  of string type that may contain non-ASCII characters.

Signed-off-by: Andreas Maier <maiera@de.ibm.com>
  • Loading branch information
@andy-maier
andy-maier committed Jul 31, 2017
1 parent 80b165c commit 8ada8d4
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 41 deletions.
3 changes: 3 additions & 0 deletions docs/changes.rst
View file
Expand Up @@ -35,6 +35,9 @@ Released: not yet
* Added support for specifying integer-typed properties of
partitions also as decimal strings inthe module input.

* Specifying input properties with Unicode characters no
longer performs unnecessary property update.

**Dependencies:**

* Increased minimum Ansible release from 2.0.0.1 to 2.2.0.0.
Expand Down
27 changes: 14 additions & 13 deletions tests/function/test_func_partition.py
View file
Expand Up @@ -884,14 +884,15 @@ def setup_crypto_adapter(self, adapter_props):
({'initial_cp_processing_weight': ("50", 50)}, True),
({'boot_logical_unit_number': '0123'}, True),
({'boot_world_wide_port_name': '0123456789abcdef'}, True),
({'boot_os_specific_parameters': 'fake'}, True),
({'boot_iso_ins_file': 'fake'}, True),
({'boot_os_specific_parameters': u'fak\u00E9'}, True),
({'boot_iso_ins_file': u'fak\u00E9'}, True),
# TODO: Add tests for SSC properties:
# ({'ssc_boot_selection': 'fake'}, True),
# allowed create+update properties:
({'description': 'fake'}, True),
({'short_name': 'fake'}, True),
({'description': u'fak\u00E9'}, True),
({'short_name': 'FAKE'}, True),
({'partition_id': '7F'}, True),
({'autogenerate_partition_id': False}, True),
({'ifl_processors': 1}, True),
Expand All @@ -909,11 +910,11 @@ def setup_crypto_adapter(self, adapter_props):
({'boot_device': 'ftp'}, True),
({'boot_timeout': 120}, True),
({'boot_timeout': ("120", 120)}, True),
({'boot_ftp_host': 'fake'}, True),
({'boot_ftp_username': 'fake'}, True),
({'boot_ftp_password': 'fake'}, True),
({'boot_ftp_insfile': 'fake'}, True),
({'boot_removable_media': 'fake'}, True),
({'boot_ftp_host': u'fak\u00E9'}, True),
({'boot_ftp_username': u'fak\u00E9'}, True),
({'boot_ftp_password': u'fak\u00E9'}, True),
({'boot_ftp_insfile': u'fak\u00E9'}, True),
({'boot_removable_media': u'fak\u00E9'}, True),
({'boot_removable_media_type': 'fake'}, True),
({'boot_configuration_selector': 4}, True),
({'boot_configuration_selector': ("4", 4)}, True),
Expand All @@ -930,11 +931,11 @@ def setup_crypto_adapter(self, adapter_props):
({'permit_des_key_import_functions': False}, True),
({'permit_aes_key_import_functions': False}, True),
# TODO: Add tests for SSC properties:
# ({'ssc_host_name': 'fake'}, True),
# ({'ssc_ipv4_gateway': 'fake'}, True),
# ({'ssc_dns_servers': 'fake'}, True),
# ({'ssc_master_userid': 'fake'}, True),
# ({'ssc_master_pw': 'fake'}, True),
# ({'ssc_host_name': u'fak\u00E9'}, True),
# ({'ssc_ipv4_gateway': u'fak\u00E9'}, True),
# ({'ssc_dns_servers': u'fak\u00E9''}, True),
# ({'ssc_master_userid': u'fak\u00E9'}, True),
# ({'ssc_master_pw': u'fak\u00E9'}, True),
])
@mock.patch("zhmc_ansible_modules.zhmc_partition.AnsibleModule",
autospec=True)
Expand Down
20 changes: 20 additions & 0 deletions zhmc_ansible_modules/utils/__init__.py
View file
Expand Up @@ -16,6 +16,8 @@
Utility functions for use by more than one Ansible module.
"""

from ansible.module_utils import six

from zhmcclient import Session
from zhmcclient_mock import FakedSession

Expand Down Expand Up @@ -281,3 +283,21 @@ def get_session(faked_session, host, userid, password):
return faked_session
else:
return Session(host, userid, password)


def to_unicode(value):
"""
Return the input value as a unicode string.
The input value may be a binary string or a unicode string, or None.
If it is a binary string, it is encoded to a unicode string using UTF-8.
"""
if isinstance(value, six.binary_type):
return value.decode('utf-8')
elif isinstance(value, six.text_type):
return value
elif value is None:
return None
else:
raise TypeError("Value is not a binary or unicode string: {!r} {}".
format(value, type(value)))
64 changes: 36 additions & 28 deletions zhmc_ansible_modules/zhmc_partition.py
View file
Expand Up @@ -22,7 +22,7 @@

from zhmc_ansible_modules.utils import Error, ParameterError, StatusError, \
stop_partition, start_partition, wait_for_transition_completion, eq_hex, \
get_hmc_auth, get_session
get_hmc_auth, get_session, to_unicode

# For information on the format of the ANSIBLE_METADATA, DOCUMENTATION,
# EXAMPLES, and RETURN strings, see
Expand Down Expand Up @@ -236,7 +236,7 @@
"""

# Dictionary of properties of partition resources, in this format:
# name: (allowed, create, update, update_while_active, eq_func)
# name: (allowed, create, update, update_while_active, eq_func, type_cast)
# where:
# name: Name of the property according to the data model, with hyphens
# replaced by underscores (this is how it is or would be specified in
Expand All @@ -253,8 +253,9 @@
# eq_func: Equality test function for two values of the property; None means
# to use Python equality.
# type_cast: Type cast function for an input value of the property; None
# means to use it directly. This can be used to convert integers provides
# as strings back into integers.
# means to use it directly. This can be used for example to convert
# integers provided as strings by Ansible back into integers (that is a
# current deficiency of Ansible).
ZHMC_PARTITION_PROPERTIES = {

# create-only properties:
Expand All @@ -264,16 +265,14 @@
'boot_network_device': (
False, False, True, True, None, None), # via boot_network_nic_name
'boot_network_nic_name': (
True, False, True, True, None, None), # artificial property
# type_cast is ignored (not needed for this property).
True, False, True, True, None, to_unicode), # artificial property
'boot_storage_device': (
False, False, True, True, None, None), # via boot_storage_hba_name
'boot_storage_hba_name': (
True, False, True, True, None, None), # artificial property
# type_cast is ignored (not needed for this property).
True, False, True, True, None, to_unicode), # artificial property
'crypto_configuration': (
True, False, False, None, None, None), # artif. props inside
# type_cast is ignored (handled specifically for domain index).
True, False, False, None, None,
None), # Contains artificial properties, type_cast ignored
'acceptable_status': (True, False, True, True, None, None),
'processor_management_enabled': (True, False, True, True, None, None),
'ifl_absolute_processor_capping': (True, False, True, True, None, None),
Expand All @@ -292,14 +291,14 @@
'initial_cp_processing_weight': (True, False, True, True, None, int),
'boot_logical_unit_number': (True, False, True, True, eq_hex, None),
'boot_world_wide_port_name': (True, False, True, True, eq_hex, None),
'boot_os_specific_parameters': (True, False, True, True, None, None),
'boot_iso_ins_file': (True, False, True, True, None, None),
'boot_os_specific_parameters': (True, False, True, True, None, to_unicode),
'boot_iso_ins_file': (True, False, True, True, None, to_unicode),
'ssc_boot_selection': (True, False, True, True, None, None),

# create+update properties:
'name': (
False, True, True, True, None, None), # provided in 'name' module parm
'description': (True, True, True, True, None, None),
'description': (True, True, True, True, None, to_unicode),
'short_name': (True, True, True, False, None, None),
'partition_id': (True, True, True, False, None, None),
'autogenerate_partition_id': (True, True, True, False, None, None),
Expand All @@ -311,11 +310,11 @@
'reserve_resources': (True, True, True, True, None, None),
'boot_device': (True, True, True, True, None, None),
'boot_timeout': (True, True, True, True, None, int),
'boot_ftp_host': (True, True, True, True, None, None),
'boot_ftp_username': (True, True, True, True, None, None),
'boot_ftp_password': (True, True, True, True, None, None),
'boot_ftp_insfile': (True, True, True, True, None, None),
'boot_removable_media': (True, True, True, True, None, None),
'boot_ftp_host': (True, True, True, True, None, to_unicode),
'boot_ftp_username': (True, True, True, True, None, to_unicode),
'boot_ftp_password': (True, True, True, True, None, to_unicode),
'boot_ftp_insfile': (True, True, True, True, None, to_unicode),
'boot_removable_media': (True, True, True, True, None, to_unicode),
'boot_removable_media_type': (True, True, True, True, None, None),
'boot_configuration_selector': (True, True, True, True, None, int),
'boot_record_lba': (True, True, True, True, None, None),
Expand All @@ -330,11 +329,11 @@
'access_diagnostic_sampling': (True, True, True, True, None, None),
'permit_des_key_import_functions': (True, True, True, True, None, None),
'permit_aes_key_import_functions': (True, True, True, True, None, None),
'ssc_host_name': (True, True, True, True, None, None),
'ssc_ipv4_gateway': (True, True, True, True, None, None),
'ssc_dns_servers': (True, True, True, True, None, None),
'ssc_master_userid': (True, True, True, True, None, None),
'ssc_master_pw': (True, True, True, True, None, None),
'ssc_host_name': (True, True, True, True, None, to_unicode),
'ssc_ipv4_gateway': (True, True, True, True, None, to_unicode),
'ssc_dns_servers': (True, True, True, True, None, to_unicode),
'ssc_master_userid': (True, True, True, True, None, to_unicode),
'ssc_master_pw': (True, True, True, True, None, to_unicode),

# read-only properties:
'object_uri': (False, False, False, None, None, None),
Expand Down Expand Up @@ -414,7 +413,7 @@ def process_properties(cpc, partition, params):
crypto_changes = None

# handle 'name' property
part_name = params['name']
part_name = to_unicode(params['name'])
create_props['name'] = part_name
# We looked up the partition by name, so we will never have to update
# the partition name
Expand Down Expand Up @@ -445,13 +444,18 @@ def process_properties(cpc, partition, params):
raise ParameterError(
"Artificial property {!r} can only be specified when the "
"partition previously exists.".format(prop_name))

hba_name = input_props[prop_name]
if type_cast:
hba_name = type_cast(hba_name)

try:
hba = partition.hbas.find(name=hba_name)
except zhmcclient.NotFound:
raise ParameterError(
"Artificial property {!r} does not name an existing HBA: "
"{!r}".format(prop_name, hba_name))

hmc_prop_name = 'boot-storage-device'
if partition.properties.get(hmc_prop_name) != hba.uri:
update_props[hmc_prop_name] = hba.uri
Expand All @@ -464,13 +468,18 @@ def process_properties(cpc, partition, params):
raise ParameterError(
"Artificial property {!r} can only be specified when the "
"partition previously exists.".format(prop_name))

nic_name = input_props[prop_name]
if type_cast:
nic_name = type_cast(nic_name)

try:
nic = partition.nics.find(name=nic_name)
except zhmcclient.NotFound:
raise ParameterError(
"Artificial property {!r} does not name an existing NIC: "
"{!r}".format(prop_name, nic_name))

hmc_prop_name = 'boot-network-device'
if partition.properties.get(hmc_prop_name) != nic.uri:
update_props[hmc_prop_name] = nic.uri
Expand Down Expand Up @@ -627,13 +636,12 @@ def process_properties(cpc, partition, params):
input_prop_value = type_cast(input_prop_value)

if partition:
current_prop_value = partition.properties.get(hmc_prop_name)
if eq_func:
equal = eq_func(partition.properties.get(hmc_prop_name),
input_prop_value,
equal = eq_func(current_prop_value, input_prop_value,
prop_name)
else:
equal = (partition.properties.get(hmc_prop_name) ==
input_prop_value)
equal = (current_prop_value == input_prop_value)
if not equal and update:
update_props[hmc_prop_name] = input_prop_value
if not update_while_active:
Expand Down

0 comments on commit 8ada8d4

Please sign in to comment.