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.

Sign up for GitHub

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

Add value key to return data #359

Closed
wants to merge 4 commits into from
Closed

Add value key to return data #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
481eeab
Add value key to return data
valkiriaaquatica May 20, 2024
a688571
update test setting to all the option
valkiriaaquatica May 20, 2024
2a3e2eb
apply black correction
valkiriaaquatica May 20, 2024
5c6e7d4
apply black to now.py
valkiriaaquatica May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions plugins/inventory/now.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@
type: int
default: 1000
version_added: 2.5.0
show_values:
description:
- Whether to include the valu' field for dictionary-type columns in returned.
type: bool
default: false

"""

Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

sysparm_display_value="all",
)
if query:
snow_query["sysparm_query"] = construct_sysparm_query(query, is_encoded_query)
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if "display_value" in inventory_hostname:
inventory_hostname = inventory_hostname["display_value"]
else:
msg = "Inventory hostname source column '{0}' is a dict but does not contain 'display_value'."
raise AnsibleParserError(msg.format(name_source))

if inventory_hostname:
host = self.inventory.add_host(inventory_hostname)
return host
Expand All @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


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

Choose a reason for hiding this comment

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

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

main_value = value.get("display_value") or value.get("value") or value
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

self.inventory.set_variable(host, k.replace(".", "_"), main_value)

if show_values:
for sub_key in ["display_value", "value"]:
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)
else:
self.inventory.set_variable(host, k.replace(".", "_"), value)

def fill_constructed(
self,
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/plugins/inventory/test_now.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"table_name", dict(sysparm_display_value="all")
)

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

"table_name", dict(sysparm_display_value="all", sysparm_query="my!=value")
)

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

"table_name", dict(sysparm_display_value="all", sysparm_fields="a,b,c")
)


Expand Down
Loading