Repository: osbuild/osbuild
Pull request #1516 (2023-12-21 18:08:35)

osbuild: error when {Device,Mount} is modified after creation
This is a drive-by change after spending some quality time with the
mount code. The `id` field of `Mount` is calculated only once and
only when creating a `Mount`. This seems slightly dangerous as
any change to an attribute after creation will not update the
id. This means two options:
1. dynamically update the `id` on changes
2. forbid changes after the `id` is calculcated

I went with (2) but happy to discuss of course but it seems more
the spirit of the class.

It also does the same change for "devices.Device"


supakeen: We spoke about: https://mypy.readthedocs.io/en/stable/final_attrs.html#final-attrs
mvo5: > We spoke about: https://mypy.readthedocs.io/en/stable/final_attrs.html#final-attrs

Thank you, this is a great suggestion! I tried it but it seems to now work for this use-case:
```python
$ cat /tmp/lala.py 
#!/usr/bin/python3

from typing import Final


class Device:
    def __init__(self, name: str):
        self.name: Final[str] = name


def main():
    dev = Device("dev-1")
    dev.name = "this shoudl error"


if __name__ == "__main__":
    main()

$ mypy /tmp/lala.py 
Success: no issues found in 1 source file
```
supakeen: Because you're running `mypy` with default settings here it is ignoring the functions without type annotations:

```
user@muja ~ € mypy /tmp/lele.py
/tmp/lele.py:13: error: Cannot assign to final attribute "name"  [misc]
Found 1 error in 1 file (checked 1 source file)
(venv) user@muja ~ € vi /tmp/lele.py
(venv) user@muja ~ € cat /tmp/lele.py
#!/usr/bin/python3

from typing import Final


class Device:
    def __init__(self, name: str) -> None:
        self.name: Final[str] = name


def main() -> None:
    dev = Device("dev-1")
    dev.name = "this shoudl error"


if __name__ == "__main__":
    main()
user@muja ~ € mypy /tmp/lele.py
/tmp/lele.py:13: error: Cannot assign to final attribute "name"  [misc]
Found 1 error in 1 file (checked 1 source file)
```

mvo5: > Because you're running `mypy` with default settings here it is ignoring the functions without type annotations:
[..]

Thank you! This is great and the example is working now. But does that mean that with our current level of mypy coverage we may not actually see an error because in the path where the modification is happening is not covered with mypy and thus ignored? I I did a quick experiment and indeed e.g. `osbuild/pipeline.py:add_device()` has no mypy attributes so here mutating "dev.name" is unoticed by mypy. However when I add (trivial) annotations like `Any` for everything `make lint` picks up the error correctly.

I wonder what we should do. I love the mypy approach (over the mixin) but it seems less pratical right now :/ OTOH this is all a bit theoretical anyway, maybe best to just close the PR and get back to final once we have more strict mypy (starting with https://github.com/osbuild/osbuild/pull/1476)

Patch:
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index fb31344d..a09f24c5 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -48,10 +48,10 @@ RPM:
           - aws/centos-stream-8-aarch64
           - aws/centos-stream-9-x86_64
           - aws/centos-stream-9-aarch64
-          - aws/rhel-8.8-ga-x86_64
-          - aws/rhel-8.8-ga-aarch64
-          - aws/rhel-9.2-ga-x86_64
-          - aws/rhel-9.2-ga-aarch64
+          - aws/rhel-8.9-ga-x86_64
+          - aws/rhel-8.9-ga-aarch64
+          - aws/rhel-9.3-ga-x86_64
+          - aws/rhel-9.3-ga-aarch64
       - RUNNER:
           - aws/rhel-8.10-nightly-x86_64
           - aws/rhel-8.10-nightly-aarch64
diff --git a/Schutzfile b/Schutzfile
index 7a39d8fc..3922bec2 100644
--- a/Schutzfile
+++ b/Schutzfile
@@ -233,47 +233,6 @@
       }
     ]
   },
-  "rhel-8.9": {
-    "repos": [
-      {
-        "file": "/etc/yum.repos.d/rhel8internal.repo",
-        "x86_64": [
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-BaseOS",
-            "name": "baseos",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-x86_64-baseos-n8.9-20240101"
-          },
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-AppStream",
-            "name": "appstream",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-x86_64-appstream-n8.9-20240101"
-          },
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-CRB",
-            "name": "crb",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-x86_64-crb-n8.9-20240101"
-          }
-        ],
-        "aarch64": [
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-BaseOS",
-            "name": "baseos",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-aarch64-baseos-n8.9-20240101"
-          },
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-AppStream",
-            "name": "appstream",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-aarch64-appstream-n8.9-20240101"
-          },
-          {
-            "title": "RHEL-8-RPMREPO-NIGHTLY-CRB",
-            "name": "crb",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el8/el8-aarch64-crb-n8.9-20240101"
-          }
-        ]
-      }
-    ]
-  },
   "rhel-8.10": {
     "repos": [
       {
@@ -315,47 +274,6 @@
       }
     ]
   },
-  "rhel-9.3": {
-    "repos": [
-      {
-        "file": "/etc/yum.repos.d/rhel9internal.repo",
-        "x86_64": [
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-BaseOS",
-            "name": "baseos",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-x86_64-baseos-n9.3-20240101"
-          },
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-AppStream",
-            "name": "appstream",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-x86_64-appstream-n9.3-20240101"
-          },
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-CRB",
-            "name": "crb",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-x86_64-crb-n9.3-20240101"
-          }
-        ],
-        "aarch64": [
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-BaseOS",
-            "name": "baseos",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-aarch64-baseos-n9.3-20240101"
-          },
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-AppStream",
-            "name": "appstream",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-aarch64-appstream-n9.3-20240101"
-          },
-          {
-            "title": "RHEL-9-RPMREPO-NIGHTLY-CRB",
-            "name": "crb",
-            "baseurl": "https://rpmrepo.osbuild.org/v2/mirror/rhvpn/el9/el9-aarch64-crb-n9.3-20240101"
-          }
-        ]
-      }
-    ]
-  },
   "rhel-9.4": {
     "repos": [
       {
@@ -452,4 +370,4 @@
       }
     ]
   }
-}
\ No newline at end of file
+}
diff --git a/osbuild/devices.py b/osbuild/devices.py
index 44541a72..8490f1e4 100644
--- a/osbuild/devices.py
+++ b/osbuild/devices.py
@@ -21,10 +21,11 @@ import stat
 from typing import Any, Dict, Optional
 
 from osbuild import host
+from osbuild.mixins import MixinImmutableID
 from osbuild.util import ctx
 
 
-class Device:
+class Device(MixinImmutableID):
     """
     A single device with its corresponding options
     """
diff --git a/osbuild/inputs.py b/osbuild/inputs.py
index 5e845df7..229c1264 100644
--- a/osbuild/inputs.py
+++ b/osbuild/inputs.py
@@ -48,7 +48,6 @@ class Input:
         self.id = self.calc_id()
 
     def calc_id(self):
-
         # NB: The input `name` is not included here on purpose since it
         # is either prescribed by the stage itself and thus not actual
         # parameter or arbitrary and chosen by the manifest generator
diff --git a/osbuild/mixins.py b/osbuild/mixins.py
new file mode 100644
index 00000000..24f09d35
--- /dev/null
+++ b/osbuild/mixins.py
@@ -0,0 +1,15 @@
+"""
+Mixin helper classes
+"""
+
+
+class MixinImmutableID:
+    """
+    Mixin to ensure that "self.id" attributes are immutable after id is set
+    """
+
+    def __setattr__(self, name, val):
+        if hasattr(self, "id"):
+            class_name = self.__class__.__name__
+            raise ValueError(f"cannot set '{name}': {class_name} cannot be changed after creation")
+        super().__setattr__(name, val)
diff --git a/osbuild/mounts.py b/osbuild/mounts.py
index 86576ad0..b938d21d 100644
--- a/osbuild/mounts.py
+++ b/osbuild/mounts.py
@@ -16,9 +16,10 @@ from typing import Dict, List
 
 from osbuild import host
 from osbuild.devices import DeviceManager
+from osbuild.mixins import MixinImmutableID
 
 
-class Mount:
+class Mount(MixinImmutableID):
     """
     A single mount with its corresponding options
     """
diff --git a/schutzbot/terraform b/schutzbot/terraform
index 125e1d86..e88c9f01 100644
--- a/schutzbot/terraform
+++ b/schutzbot/terraform
@@ -1,1 +1,1 @@
-40e5f0c5d7da48ef4521e051bcf0ceaab429d02f
+0caf5afd068c94b7c39419bb1a2249d1e1dae6b2


Test patch:
diff --git a/test/mod/test_mixins.py b/test/mod/test_mixins.py
new file mode 100644
index 00000000..e793ad7e
--- /dev/null
+++ b/test/mod/test_mixins.py
@@ -0,0 +1,32 @@
+from unittest.mock import Mock
+
+import pytest
+
+from osbuild.devices import Device
+from osbuild.meta import ModuleInfo
+from osbuild.mounts import Mount
+
+
+def test_mount_immutable_mixin():
+    info = Mock(spec=ModuleInfo)
+    info.name = "some-name"
+    device = Mock(spec=ModuleInfo)
+    device.id = "some-id"
+    partition = 1
+    target = "/"
+    opts = {"opt1": 1}
+    mnt = Mount("name", info, device, partition, target, opts)
+    with pytest.raises(ValueError) as e:
+        mnt.name = "new-name"
+    assert str(e.value) == "cannot set 'name': Mount cannot be changed after creation"
+
+
+def test_device_immutable_mixins():
+    info = Mock(spec=ModuleInfo)
+    info.name = "some-name"
+    parent = None
+    opts = {"opt1": 1}
+    dev = Device("name", info, parent, opts)
+    with pytest.raises(ValueError) as e:
+        dev.name = "new-name"
+    assert str(e.value) == "cannot set 'name': Device cannot be changed after creation"