inventory: Add warning for invalid priority values (#86114)

* Handle ValueError raised when user set invalid priority values
* Update tests to work with Pytest

Signed-off-by: Abhijeet Kasurde <Akasurde@redhat.com>
Co-authored-by: Mannu Silva <wise.tent4987@fastmail.com>

changelogs/fragments/group-priority-warning.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - group - Add warning message when invalid priority values are provided to Group.set_priority() method (https://github.com/ansible/ansible/pull/85468).

lib/ansible/inventory/group.py

@@ -1,19 +1,6 @@
 # (c) 2012-2014, Michael DeHaan <michael.dehaan@gmail.com>
-#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
-# This file is part of Ansible
+
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 from __future__ import annotations
 
 import typing as t
@@ -227,7 +214,7 @@ class Group:
             Display().deprecated(msg=f'Accepting inventory variable with invalid name {key!r}.', version='2.23', help_text=ex._help_text, obj=ex.obj)
 
         if key == 'ansible_group_priority':
-            self.set_priority(int(value))
+            self.set_priority(value)
         else:
             if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
                 self.vars = combine_vars(self.vars, {key: value})
@@ -266,6 +253,10 @@ class Group:
     def set_priority(self, priority: int | str) -> None:
         try:
             self.priority = int(priority)
-        except TypeError:
+        except (TypeError, ValueError) as e:
-            # FIXME: warn about invalid priority
+            display.error_as_warning(
-            pass
+                msg=f"Invalid priority value '{priority}' for group '{self.name}'."
+                    "Setting priority to default value.",
+                exception=e,
+            )
+            # Keep the existing priority value when conversion fails

test/units/inventory/test_group.py

@@ -1,42 +1,27 @@
 # Copyright 2018 Alan Rominger <arominge@redhat.com>
-#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
-# This file is part of Ansible
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
 from __future__ import annotations
 
-import unittest
+import pytest
 
 from ansible.inventory.group import Group
 from ansible.inventory.host import Host
 from ansible.errors import AnsibleError
 
 
-class TestGroup(unittest.TestCase):
+def test_depth_update():
-
-    def test_depth_update(self):
     A = Group('A')
     B = Group('B')
     Z = Group('Z')
     A.add_child_group(B)
     A.add_child_group(Z)
-        self.assertEqual(A.depth, 0)
+    assert A.depth == 0
-        self.assertEqual(Z.depth, 1)
+    assert Z.depth == 1
-        self.assertEqual(B.depth, 1)
+    assert B.depth == 1
+
 
-    def test_depth_update_dual_branches(self):
+def test_depth_update_dual_branches():
     alpha = Group('alpha')
     A = Group('A')
     alpha.add_child_group(A)
@@ -48,17 +33,18 @@ class TestGroup(unittest.TestCase):
     B.add_child_group(beta)
     Z.add_child_group(beta)
 
-        self.assertEqual(alpha.depth, 0)  # apex
+    assert alpha.depth == 0  # apex
-        self.assertEqual(beta.depth, 3)  # alpha -> A -> B -> beta
+    assert beta.depth == 3  # alpha -> A -> B -> beta
 
     omega = Group('omega')
     omega.add_child_group(alpha)
 
     # verify that both paths are traversed to get the max depth value
-        self.assertEqual(B.depth, 3)  # omega -> alpha -> A -> B
+    assert B.depth == 3  # omega -> alpha -> A -> B
-        self.assertEqual(beta.depth, 4)  # B -> beta
+    assert beta.depth == 4  # B -> beta
 
-    def test_depth_recursion(self):
+
+def test_depth_recursion():
     A = Group('A')
     B = Group('B')
     A.add_child_group(B)
@@ -66,19 +52,21 @@ class TestGroup(unittest.TestCase):
     A.parent_groups.append(B)
     B.child_groups.append(A)
     # can't update depths of groups, because of loop
-        with self.assertRaises(AnsibleError):
+    with pytest.raises(AnsibleError):
         B._check_children_depth()
 
-    def test_loop_detection(self):
+
+def test_loop_detection():
     A = Group('A')
     B = Group('B')
     C = Group('C')
     A.add_child_group(B)
     B.add_child_group(C)
-        with self.assertRaises(AnsibleError):
+    with pytest.raises(AnsibleError):
         C.add_child_group(A)
 
-    def test_direct_host_ordering(self):
+
+def test_direct_host_ordering():
     """Hosts are returned in order they are added
     """
     group = Group('A')
@@ -91,7 +79,8 @@ class TestGroup(unittest.TestCase):
         expected_hosts.append(h)
     assert group.get_hosts() == expected_hosts
 
-    def test_sub_group_host_ordering(self):
+
+def test_sub_group_host_ordering():
     """With multiple nested groups, asserts that hosts are returned
     in deterministic order
     """
@@ -105,7 +94,8 @@ class TestGroup(unittest.TestCase):
         expected_hosts.append(host)
     assert top_group.get_hosts() == expected_hosts
 
-    def test_populates_descendant_hosts(self):
+
+def test_populates_descendant_hosts():
     A = Group('A')
     B = Group('B')
     C = Group('C')
@@ -114,12 +104,13 @@ class TestGroup(unittest.TestCase):
     A.add_child_group(B)  # B is child of A
     B.add_child_group(C)  # C is descendant of A
     A.add_child_group(B)
-        self.assertEqual(set(h.groups), set([C, B, A]))
+    assert set(h.groups) == set([C, B, A])
     h2 = Host('h2')
     C.add_host(h2)
-        self.assertEqual(set(h2.groups), set([C, B, A]))
+    assert set(h2.groups) == set([C, B, A])
 
-    def test_ancestor_example(self):
+
+def test_ancestor_example():
     # see docstring for Group._walk_relationship
     groups = {}
     for name in ['A', 'B', 'C', 'D', 'E', 'F']:
@@ -134,14 +125,15 @@ class TestGroup(unittest.TestCase):
     groups['D'].add_child_group(groups['F'])
     groups['E'].add_child_group(groups['F'])
 
-        self.assertEqual(
+    assert (
-            set(groups['F'].get_ancestors()),
+        set(groups['F'].get_ancestors()) ==
         set([
             groups['A'], groups['B'], groups['C'], groups['D'], groups['E']
         ])
     )
 
-    def test_ancestors_recursive_loop_safe(self):
+
+def test_ancestors_recursive_loop_safe():
     """
     The get_ancestors method may be referenced before circular parenting
     checks, so the method is expected to be stable even with loops
@@ -151,4 +143,29 @@ class TestGroup(unittest.TestCase):
     A.parent_groups.append(B)
     B.parent_groups.append(A)
     # finishes in finite time
-        self.assertEqual(A.get_ancestors(), set([A, B]))
+    assert A.get_ancestors() == set([A, B])
+
+
+@pytest.mark.parametrize("priority, expected", [
+    pytest.param(5, 5, id="int"),
+    pytest.param('10', 10, id="string"),
+    pytest.param(-1, -1, id="negative number"),
+])
+def test_set_priority_valid_values(priority, expected):
+    """Test that valid priority value is set"""
+    group = Group('test_group')
+    group.set_priority(priority)
+    assert group.priority == expected
+
+
+@pytest.mark.parametrize("priority, expected", [
+    pytest.param('invalid', 1, id="invalid string"),
+    pytest.param(None, 1, id="None"),
+    pytest.param({'key': 'value'}, 1, id="dict"),
+    pytest.param(['item'], 1, id="list")
+])
+def test_set_priority_invalid_values(priority, expected):
+    """Test that invalid priority value is handled gracefully with warnings"""
+    group = Group('test_group')
+    group.set_priority(priority)
+    assert group.priority == expected