Fix idempotency of mount strings (#193)

* Join mount strings correctly.

Previously this function would prepend an extra comma to the front of the result in some cases. Now the function does a single join so avoids having to handle these edge cases.

* Actually test the test

---------

Co-authored-by: Ryan Smith <[email protected]>

Author: rfsmi

plugins/modules/proxmox.py

@@ -1559,9 +1559,10 @@ def build_volume(self, vmid, node, key, storage=None, volume=None, host_path=Non
                         volume=volume,
                     ),
                 )
-            vol_string = "{storage}:{volume},size={size}".format(
-                storage=storage, volume=volume, size=size
-            )
+            vol_parts = [
+                "{storage}:{volume}".format(storage=storage, volume=volume),
+                "size={size}".format(size=size),
+            ]
         # 2. If volume not defined (but storage is), check if it exists
         elif storage is not None:
             proxmox_node = self.proxmox_api.nodes(
@@ -1570,9 +1571,10 @@ def build_volume(self, vmid, node, key, storage=None, volume=None, host_path=Non
             try:
                 vol = proxmox_node.lxc(vmid).get("config").get(key)
                 volume = self.parse_disk_string(vol).get("volume")
-                vol_string = "{storage}:{volume},size={size}".format(
-                    storage=storage, volume=volume, size=size
-                )
+                vol_parts = [
+                    "{storage}:{volume}".format(storage=storage, volume=volume),
+                    "size={size}".format(size=size),
+                ]
 
             # If not, we have proxmox create one using the special syntax
             except Exception:
@@ -1582,36 +1584,32 @@ def build_volume(self, vmid, node, key, storage=None, volume=None, host_path=Non
                     )
                 elif size.endswith("G"):
                     size = size.rstrip("G")
-                    vol_string = "{storage}:{size}".format(storage=storage, size=size)
+                    vol_parts = ["{storage}:{size}".format(storage=storage, size=size)]
                 else:
                     raise ValueError(
                         "Size must be provided in GiB for storage-backed volume creation. Convert it to GiB or allocate a new storage manually."
                     )
         # 3. If we have a host_path, we don't have storage, a volume, or a size
         # Then we don't have to do anything, just build and return the vol_string
         elif host_path is not None:
-            vol_string = ""
+            vol_parts = []
         else:
             raise ValueError(
                 "Could not build a valid volume string. One of volume, storage, or host_path must be provided."
             )
 
         if host_path is not None:
-            vol_string += "," + host_path
+            vol_parts += [host_path]
 
         if mountpoint is not None:
-            vol_string += ",mp={}".format(mountpoint)
+            vol_parts += ["mp={}".format(mountpoint)]
 
         if options is not None:
-            vol_string += "," + ",".join(
-                ["{0}={1}".format(k, v) for k, v in options.items()]
-            )
+            vol_parts += ["{0}={1}".format(k, v) for k, v in options.items()]
 
         if kwargs:
-            vol_string += "," + ",".join(
-                ["{0}={1}".format(k, v) for k, v in kwargs.items()]
-            )
-        return {key: vol_string}
+            vol_parts += ["{0}={1}".format(k, v) for k, v in kwargs.items()]
+        return {key: ",".join(vol_parts)}
 
     def get_lxc_resource(self, vmid, hostname):
         if not vmid and not hostname:

tests/unit/plugins/modules/test_proxmox_lxc.py

@@ -0,0 +1,33 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (c) 2025, Ryan Smith <[email protected]>
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+from unittest.mock import MagicMock, patch
+from ansible.module_utils.basic import AnsibleModule
+from ansible_collections.community.proxmox.plugins.modules import proxmox
+from ansible_collections.community.proxmox.plugins.module_utils.proxmox import ProxmoxAnsible
+from ansible.module_utils.compat.version import LooseVersion
+
+
+@patch.object(ProxmoxAnsible, "__init__", return_value=None)
+@patch.object(ProxmoxAnsible, "version", return_value=LooseVersion("4.0"))
+@patch.object(ProxmoxAnsible, "proxmox_api", create=True)
+@patch.object(ProxmoxAnsible, "module", create=True)
+def test_mount_formatting(mock_api, *_):
+    """Test the process_mount_keys method correctly formats mounts."""
+    lxc_ansible = proxmox.ProxmoxLxcAnsible(MagicMock(spec=AnsibleModule))
+    mount_volumes = [{
+        'host_path': '/mnt/dir',
+        'mountpoint': 'mnt/dir',
+        'id': 'mp0',
+        'storage': None,
+        'volume': None,
+        'size': None,
+        'options': None,
+    }]
+    mounts = lxc_ansible.process_mount_keys(
+        100, "my-node", None, mount_volumes
+    )
+    assert mounts == {'mp0': '/mnt/dir,mp=mnt/dir'}