Skip to content

Grain.item issue # 31427 removes duplicates in item returned lists #68289

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

Open
wants to merge 2 commits into
base: 3006.x
Choose a base branch
from
+23 −0
Open

Grain.item issue # 31427 removes duplicates in item returned lists #68289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbdb4bd
initial commit
bertdawg76 Aug 26, 2025
46e41f9
Merge branch '3006.x' into grain#31427
bertdawg76 Sep 3, 2025
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
1 change: 1 addition & 0 deletions changelog/31427.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes duplicate roles returning in list from grains.item call
4 changes: 4 additions & 0 deletions salt/modules/grains.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@ def item(*args, **kwargs):
for arg, func in _SANITIZERS.items():
if arg in ret:
ret[arg] = func(ret[arg])

if isinstance(ret[arg], list):
unique_ret = list(dict.fromkeys(ret[arg]))
ret[arg] = unique_ret
Comment on lines +210 to +212
Copy link
Contributor

@bdrx312 bdrx312 Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This does not seem correct as a change for any generic grain that has a list value. It is conceivable to me that a grain could intentionally have a list that has items that have the same value--especially with a custom grain. Nothing I have read states that the value of a grain must be a set of unique values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. The referenced issue relates to the state module adding duplicates, not the execution one listing duplicates.

It should have already been fixed in #31471 though, is it still an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

#31471 itself seems to indicate that duplicate items in the list is unexpected behavior. I can't think of a scenario where it would be desirable. That being said, this PR does not seem to fix that issue.

Copy link
Collaborator

@barneysowood barneysowood Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are definitely cases with existing grains where it is necessary to have duplicate items:

$ salt-call grains.get pythonversion
local:
    - 3
    - 10
    - 18
    - final
    - 0

What would have happened for Python 3.10.10?

More generally, it is a list, we should keep python list semantics, ie an ordered collection of items which aren't necessarily unique.

I also don't think that this conflicts with the behaviour of the grains.present - that is to ensure a value is present in the list, if it is already present, don't add it again.. I don't think that precludes having lists that have duplicate items, it's just not what that function is for.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. The referenced issue relates to the state module adding duplicates, not the execution one listing duplicates.

It should have already been fixed in #31471 though, is it still an issue?

Looks like it is, although possibly slightly different behaviour, see

Copy link
Contributor

Choose a reason for hiding this comment

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

I stand corrected on allowing duplicate items in a grain. Perhaps this isn't a bug then.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @barneysowood points out, the bug probably lies in the state code

return ret


Expand Down
6 changes: 6 additions & 0 deletions tests/integration/files/conf/minion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ integration.test: True

# Grains addons
grains:
roles:
- one
- two
- three
- three
- four
test_grain: cheese
script: grail
alot: many
Expand Down
12 changes: 12 additions & 0 deletions tests/pytests/integration/modules/grains/test_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ def test_item(salt_call_cli, minion_test_grain):
assert ret.data["test_grain"] == minion_test_grain


def test_item_no_duplicate(salt_call_cli):
"""
grains.item
"""
expected = ["one", "two", "three", "four"]
ret = salt_call_cli.run("grains.item", "roles")
assert ret.returncode == 0
assert ret.data
assert isinstance(ret.data, dict)
assert ret.data["roles"] == expected


def test_ls(salt_call_cli, grains):
"""
grains.ls
Expand Down
Loading