[bugfix] Added fix to support ansible-core 2.19 to the aci_rest module (DCNE-504) #806

sajagana · 2025-09-09T14:35:20Z

No description provided.

samiib · 2025-09-09T22:43:20Z

plugins/modules/aci_rest.py

+        return {k: convert_payload_values_to_str(v) for k, v in data.items()}
+    elif isinstance(data, list):
+        return [convert_payload_values_to_str(item) for item in data]
+    elif isinstance(data, int) or isinstance(data, bool) or isinstance(data, float):


Does this handle JSON Null types? Looks like it does as null in JSON won't be converted to a string.
Would it be possible to add a test when a JSON value is explicitly set to null?

akinross · 2025-09-10T06:41:10Z

plugins/modules/aci_rest.py

+  convert_values_to_str:
+    description:
+    - When O(convert_values_to_str=true), this attribute converts input object values to strings.
+    - This ensures compatibility with Jinja2 version 3.1.6 and above.


perhaps mention the ansible version here also which introduced the change?

akinross · 2025-09-10T06:42:08Z

plugins/modules/aci_rest.py

    description:
    - The page number to return.
    type: int
+  convert_values_to_str:


should we use full name, and do aliases for abbreviated versions?

we still use str and int, should we use full naming? Also as mentioned in you function naming I think we should align the two.

akinross · 2025-09-10T06:43:37Z

plugins/modules/aci_rest.py

+
+
+def convert_payload_values_to_str(data):
+    if data in [None, ""]:


When a None value is provided should this be set to an empty string? In our collection for None we typically ignore the value being added to the payload, why diverge form this behaviour here?

This could unintentionally lead to values being overwritten on an existing MO to an empty string, which should be ignored. Like a description attribute for example.

Also I do not understand why look for empty string, this is just a valid value that requires no translation

I agree, I don't think we should be translating JSON Null to an empty string. Instead I think null values should be ignored in the request body. Although I am unsure if ACI actually accepts null json as a valid option anywhere.

akinross · 2025-09-10T07:09:48Z

plugins/modules/aci_rest.py

    description:
    - The page number to return.
    type: int
+  convert_values_to_str:


do we need this option, when apic typically only allows string values? there is one exception in aci rest test

This parameter is required to control the convertion, so I keep it for now.

akinross · 2025-09-10T07:40:25Z

plugins/modules/aci_rest.py

+        return {k: convert_payload_values_to_str(v) for k, v in data.items()}
+    elif isinstance(data, list):
+        return [convert_payload_values_to_str(item) for item in data]
+    elif isinstance(data, int) or isinstance(data, bool) or isinstance(data, float):


how are boolean values handled in 2.18 vs 2.19? Is this required to convert to string?

No, I changed the logic.

samiib · 2025-09-10T11:12:16Z

plugins/modules/aci_rest.py

+
+
+def convert_payload_values_to_str(data):
+    if data in [None, ""]:


I agree, I don't think we should be translating JSON Null to an empty string. Instead I think null values should be ignored in the request body. Although I am unsure if ACI actually accepts null json as a valid option anywhere.

sajagana · 2025-09-11T05:59:54Z

Hi Team,

FYI, there is a difference between Ansible Core 2.18 and 2.19 when using a list of YAML/JSON values to populate task values in an Ansible playbook.

Sample task:

- name: Task to test value conversion
  cisco.aci.aci_rest:
    output_level: debug
    content:
      lacpIfPol:
        attributes:
          emptyString: "{{ item.emptyString }}"
          "null": "{{ item.null }}"
          bool: "{{ item.bool }}"
          int: "{{ item.int }}"
          float: "{{ item.float }}"
          string: "{{ item.string }}"
          unspecified: "{{ item.unspecified }}"
  loop: "{{ input_values | list }}"
  vars:
    input_values:
      - emptyString: ""
        "null": null
        bool: true
        int: 101
        float: 1.44
        string: Ansible
        unspecified:

Type	Ansible Core 2.18	Ansible Core 2.19
Bool	True	True
Empty String	""	""
Float	"1.44"	1.44
Int	"101"	101
String	"Ansible"	"Ansible"
Null	""	null
Unspecified Attribute Value	""	null

But, no differences are observed when the values are used directly within the task.

- name: Task to test value conversion
  cisco.aci.aci_rest:
    output_level: debug
    content:
      lacpIfPol:
        attributes:
          emptyString: ""
          "null": null
          bool: false
          int: 101
          float: 1.44
          string: Ansible
          unspecified:

Key	Value
emptyString	''
bool	False
int	101
float	1.44
string	Ansible
null	None
unspecified	None

samiib · 2025-09-15T06:06:51Z

plugins/modules/aci_rest.py

+        return {k: convert_payload_values_to_str(v) for k, v in data.items()}
+    elif isinstance(data, list):
+        return [convert_payload_values_to_str(item) for item in data]
+    elif (isinstance(data, int) and not isinstance(data, bool)) or isinstance(data, float):


Does this mean only int and float types are converted to strings?
I am curious how the not bool condition is used here?

In Python, the bool type is a subclass of the int type. Therefore, to prevent a Boolean from being converted to its string representation ("True" or "False"), we should explicitly check its type using not isinstance(data, bool).

samiib

LGTM. However, looks like there are other outstanding comments.

gmicol · 2025-09-19T15:42:38Z

plugins/modules/aci_rest.py

+  convert_values_to_str:
+    description:
+    - When O(convert_values_to_str=true), this attribute converts input object values to strings.
+    - This ensures compatibility with Jinja2 version 3.1.6 and above.


As Akini commented, we should mention the Ansible version here as well

akinross · 2025-09-23T08:17:53Z

tests/integration/targets/aci_rest/tasks/yaml_inline.yml

-    validate_certs: '{{ aci_validate_certs | default(false) }}'
-    use_ssl: '{{ aci_use_ssl | default(true) }}'
-    use_proxy: '{{ aci_use_proxy | default(true) }}'
+    host: "{{ aci_hostname }}"


if we are changing this, why not set it as fact 1 time, and use those with <<?

alternatively we can also set/leverage defaults when used on every task

tests/integration/targets/aci_rest/tasks/error_handling.yml

plugins/modules/aci_rest.py

shrsr

LGTM

shrsr

LGTM

gmicol

LGTM

anvitha-jain

LGTM

akinross

LGTM

lhercot · 2025-10-03T18:13:28Z

Please resolve conflict

…s_to_str function

…e convert_payload_values_to_str function in aci_rest module

…and renamed the function to recursive_convert_none_int_float_to_str_in_dict_list

… the aci_rest module

…o str converstion logic

…to normalize_payload_values in the aci_rest module

shrsr

LGTM

akinross

LGTM

samiib

LGTM

anvitha-jain

LGTM

gmicol

LGTM

lhercot

LGTM

sajagana requested review from akinross, anvitha-jain, gmicol, lhercot, samiib and shrsr as code owners September 9, 2025 14:35

sajagana linked an issue Sep 9, 2025 that may be closed by this pull request

cisco.aci.aci_rest fails in ansible-core 2.19.0 (DCNE-504) #799

Closed

github-actions bot changed the title ~~[bugfix] Added fix to support ansible-core 2.19 to the aci_rest module~~ [bugfix] Added fix to support ansible-core 2.19 to the aci_rest module (DCNE-504) Sep 9, 2025

sajagana force-pushed the 504_aci_rest branch from f674cda to d092cbb Compare September 9, 2025 15:06

samiib requested changes Sep 9, 2025

View reviewed changes

sajagana requested a review from samiib September 10, 2025 06:06

akinross requested changes Sep 10, 2025

View reviewed changes

akinross reviewed Sep 10, 2025

View reviewed changes

samiib requested changes Sep 10, 2025

View reviewed changes

samiib requested changes Sep 15, 2025

View reviewed changes

sajagana requested review from akinross and samiib September 17, 2025 04:20

samiib previously approved these changes Sep 18, 2025

View reviewed changes

gmicol requested changes Sep 19, 2025

View reviewed changes

sajagana dismissed samiib’s stale review via 17b0811 September 23, 2025 07:37

sajagana force-pushed the 504_aci_rest branch from 17b0811 to d6adc5b Compare September 23, 2025 07:46

sajagana requested review from gmicol and samiib September 23, 2025 08:00

akinross requested changes Sep 23, 2025

View reviewed changes

sajagana requested a review from akinross September 23, 2025 14:05

shrsr requested changes Sep 23, 2025

View reviewed changes

tests/integration/targets/aci_rest/tasks/error_handling.yml Show resolved Hide resolved

plugins/modules/aci_rest.py Outdated Show resolved Hide resolved

shrsr self-requested a review September 23, 2025 17:47

shrsr previously approved these changes Sep 23, 2025

View reviewed changes

sajagana requested review from akinross and samiib October 1, 2025 11:31

shrsr previously approved these changes Oct 1, 2025

View reviewed changes

gmicol previously approved these changes Oct 1, 2025

View reviewed changes

anvitha-jain previously approved these changes Oct 1, 2025

View reviewed changes

akinross previously approved these changes Oct 2, 2025

View reviewed changes

sajagana added 7 commits October 8, 2025 19:27

[bugfix] Added fix to support ansible-core 2.19 to the aci_rest module

23757c7

[ignore] Added None value check to the aci_rest convert_payload_value…

c6da566

…s_to_str function

[ignore] Removed empty string check and bool to str converstion in th…

b2f6b79

…e convert_payload_values_to_str function in aci_rest module

[ignore] Moved the convert values to string function logic to aci.py …

8bae166

…and renamed the function to recursive_convert_none_int_float_to_str_in_dict_list

[ignore] Added ACI_CONVERT_VALUES_TO_STR environment variable support…

64b9e88

… the aci_rest module

[ignore] Reverted auto formatted tests and changed the aci_rest int t…

d39e02f

…o str converstion logic

[ignore] Changed the module attribute name from primitives_to_string …

bafa1aa

…to normalize_payload_values in the aci_rest module

sajagana dismissed stale reviews from akinross, anvitha-jain, gmicol, and shrsr via bafa1aa October 8, 2025 14:00

sajagana force-pushed the 504_aci_rest branch from d850675 to bafa1aa Compare October 8, 2025 14:00

sajagana requested review from akinross, anvitha-jain, gmicol and shrsr October 8, 2025 14:02

shrsr approved these changes Oct 8, 2025

View reviewed changes

akinross approved these changes Oct 9, 2025

View reviewed changes

samiib approved these changes Oct 9, 2025

View reviewed changes

anvitha-jain approved these changes Oct 9, 2025

View reviewed changes

gmicol approved these changes Oct 9, 2025

View reviewed changes

lhercot approved these changes Oct 9, 2025

View reviewed changes

lhercot merged commit 9365b5c into CiscoDevNet:master Oct 9, 2025
27 of 28 checks passed



		def convert_payload_values_to_str(data):
		if data in [None, ""]:

[bugfix] Added fix to support ansible-core 2.19 to the aci_rest module (DCNE-504) #806

[bugfix] Added fix to support ansible-core 2.19 to the aci_rest module (DCNE-504) #806

Uh oh!

Conversation

sajagana commented Sep 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akinross Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajagana commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samiib left a comment

Choose a reason for hiding this comment

Uh oh!

gmicol Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

gmicol left a comment

Choose a reason for hiding this comment

Uh oh!

anvitha-jain left a comment

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

lhercot commented Oct 3, 2025

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

akinross Sep 10, 2025 •

edited

Loading

sajagana commented Sep 11, 2025 •

edited

Loading

gmicol Sep 19, 2025 •

edited

Loading