Add value key to return data #359

valkiriaaquatica · 2024-05-20T10:33:13Z

SUMMARY

When receiving objetct that contain a dictionary with display_value, link and value, just the display_value was returned, now it returns also the value that is the sys_id.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

now.py

ADDITIONAL INFORMATION

In relation with this issue #317 , with the actual plugin inventory it just returns the display_value of objects, with the change the value (the sys_id) is also returned.
This has been usefull to me when working with server inventories I wanted to get the email associated from the user (columns assigned_to and owneed_by) but just the name was returned so I couldn't resolve it to email (excepto for using LDAP modules), so with the change now the sys_id of the Persona (object) is returned so I can use the api_info module fetching with the sys_id and get the data like in my case the emaill

Before with one example


    "_meta": {
        "hostvars": {
            "Poller": {
                "assigned_to": "Mathuew",
                "owned_by": "Steven",
                "u_server_id": "i-123456"
            }
        }
    },

plugin: servicenow.itsm.now
table: cmdb_ci_server
sysparm_query: 'name=Poller'
columns: assigned_to, owned_by, u_server_id

AFTER

    "_meta": {
        "hostvars": {
            "Poller": {
                "assigned_to": "Mathuew",
                "assigned_to_value": "123456789101123145",
                "owned_by": "Steven",
                "owned_by_value": "987654321",
                "u_server_id": "i-123456"
            }
        }
    },

Inventory adding the show_values argument

plugin: servicenow.itsm.now
table: cmdb_ci_server
sysparm_query: 'name=Poller'
columns: assigned_to, owned_by, u_server_id
show_values: true

When receiving objetct that contain a dictionary with display_value, link and value, just the display_value was returned, now it returns also the value that is the sys_id

softwarefactory-project-zuul · 2024-05-20T10:46:43Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/6fa2060a0d914b2285021ea9ff5c169f

✔️ ansible-galaxy-importer SUCCESS in 3m 50s
✔️ build-ansible-collection SUCCESS in 8m 23s

valkiriaaquatica · 2024-05-20T11:03:49Z

Hi,
I'am checking the CI errors and there is an error in black format in test_service_catalog_info.py which is not in my PR, shoudl i fix it?
The other error in unit _"AttributeError: 'InventoryModule' object has no attribute 'load_name'" which in a first seen I do no know how to fix it

Thanks in advance for the help

softwarefactory-project-zuul · 2024-05-20T11:11:19Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/1080d90151ea496dac96ec3d22ed0314

✔️ ansible-galaxy-importer SUCCESS in 5m 31s
✔️ build-ansible-collection SUCCESS in 7m 37s

tupyy · 2024-05-21T15:00:40Z

Hi, I'am checking the CI errors and there is an error in black format in test_service_catalog_info.py which is not in my PR, shoudl i fix it?

Thanks in advance for the help

No. the lint is broken. it's checks out the main branch just lint your code with the black version in the workflow.

tupyy · 2024-05-22T07:44:16Z

plugins/inventory/now.py

@@ -364,7 +369,7 @@ def construct_sysparm_query(query, is_encoded_query):
 def fetch_records(table_client, table, query, fields=None, is_encoded_query=False):
    snow_query = dict(
        # Make references and choice fields human-readable
-        sysparm_display_value=True,


Please use the value of show_values to set this parameter. You risk breaking the inventories of other users.

By default it needs to be True. Change it to all only if show_values is true

tupyy · 2024-05-22T07:47:16Z

plugins/inventory/now.py

@@ -426,6 +431,13 @@ def add_host(self, record, name_source):
            raise AnsibleParserError(msg.format(name_source))

        inventory_hostname = record[name_source]
+        if isinstance(inventory_hostname, dict):


You don't need this. You already set default: false for display_value. Even if the user does not explicitly use display_value, it will always be present in the module.

tupyy · 2024-05-22T07:47:41Z

plugins/inventory/now.py

+                        sub_value = value.get(sub_key)
+                        if sub_value and sub_value != main_value:
+                            self.inventory.set_variable(
+                                host, f"{k}_{sub_key}", sub_value


please use format. We need to be py2.8 compatible.

tupyy · 2024-05-22T07:49:04Z

tests/unit/plugins/inventory/test_now.py

@@ -50,21 +50,21 @@ def test_no_query(self, table_client):
        now.fetch_records(table_client, "table_name", None)

        table_client.list_records.assert_called_once_with(
-            "table_name", dict(sysparm_display_value=True)


the tests need to take into account the new option display_value and not hard coded value for sysparm_display_value

tupyy · 2024-05-22T07:49:15Z

tests/unit/plugins/inventory/test_now.py

        )

    def test_query(self, table_client):
        now.fetch_records(table_client, "table_name", [dict(my="!= value")])

        table_client.list_records.assert_called_once_with(
-            "table_name", dict(sysparm_display_value=True, sysparm_query="my!=value")


same as above

tupyy · 2024-05-22T07:49:26Z

tests/unit/plugins/inventory/test_now.py

        )

    def test_no_query_with_fields(self, table_client):
        now.fetch_records(table_client, "table_name", None, fields=["a", "b", "c"])

        table_client.list_records.assert_called_once_with(
-            "table_name", dict(sysparm_display_value=True, sysparm_fields="a,b,c")


same as above

tupyy · 2024-05-22T07:57:10Z

plugins/inventory/now.py

@@ -442,8 +454,23 @@ def set_hostvars(self, host, record, columns):
                "Invalid column names: {0}.".format(", ".join(missing))
            )

+        show_values = self.get_option("show_values")


please pass show_values to set_hostvars instead of looking into the module.

I will advise passing a default value like def set_hostvars(self, host, record, columns, show_values=False)

tupyy · 2024-05-22T07:58:47Z

plugins/inventory/now.py

        for k in columns:
-            self.inventory.set_variable(host, k.replace(".", "_"), record[k])
+            value = record.get(k)
+            if isinstance(value, dict):


what if the value is not a dict? Where do you set the value to host_var if value is str?

tupyy · 2024-05-22T08:32:38Z

plugins/inventory/now.py

-            self.inventory.set_variable(host, k.replace(".", "_"), record[k])
+            value = record.get(k)
+            if isinstance(value, dict):
+                main_value = value.get("display_value") or value.get("value") or value


either use display_value or value but like this is dangerous. You imply that you can use the whole dict if display_value or value are not present but I don't think you need this.

The problem with this code is that you hardcoded sysparam_display_value to all, which means you'll always get display_value and value therefore the first and second or are useless.
Also, in the if block the for is useless because, as you'll always have a value for display_value, you only need the value value.
Am I right or I'm missing something here?

tupyy

Hi! Thank you for your work. We do appreciate the effort you put into this.

We really need this option to change sysparm_display_value but I think we need to make it more flexible like show_values has 3 possible values: true, false, all (basically maps the sysparm_display_value).
In this way, the user can choose what value should be added to the inventory.

Another thing, you need tests: unit tests and integration. Also, you need to document your change with a fragment.

@mhjacks WDYT

Add value key to return data

481eeab

When receiving objetct that contain a dictionary with display_value, link and value, just the display_value was returned, now it returns also the value that is the sys_id

valkiriaaquatica added 3 commits May 20, 2024 12:49

update test setting to all the option

a688571

apply black correction

2a3e2eb

apply black to now.py

5c6e7d4

tupyy self-requested a review May 21, 2024 15:01

tupyy self-assigned this May 21, 2024

tupyy reviewed May 22, 2024

View reviewed changes

tupyy requested changes May 22, 2024

View reviewed changes

valkiriaaquatica closed this by deleting the head repository Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add value key to return data #359

Add value key to return data #359

valkiriaaquatica commented May 20, 2024

softwarefactory-project-zuul bot commented May 20, 2024

valkiriaaquatica commented May 20, 2024

softwarefactory-project-zuul bot commented May 20, 2024

tupyy commented May 21, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy May 22, 2024

tupyy left a comment •

edited

Loading

Add value key to return data #359

Add value key to return data #359

Conversation

valkiriaaquatica commented May 20, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Before with one example

softwarefactory-project-zuul bot commented May 20, 2024

valkiriaaquatica commented May 20, 2024

softwarefactory-project-zuul bot commented May 20, 2024

tupyy commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tupyy left a comment • edited Loading

Choose a reason for hiding this comment

tupyy left a comment •

edited

Loading