launchpad-reviewers team mailing list archive

[Colin Watson <mp+413099@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lpcraft:constrain-names into lpcraft:main.

Commit message:
Constrain job, series, and architecture names

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lpcraft/+git/lpcraft/+merge/413099

To avoid confusion elsewhere (for example with using these names on command lines), it seems like a good idea to constrain job, series, and architecture names to use a limited syntax that makes sense for unique identifiers.  I based this on the syntax permitted for names in Launchpad, although it isn't quite identical since I think people are likely to want to use underscores here and there's no good reason to forbid that.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lpcraft:constrain-names into lpcraft:main.

diff --git a/lpcraft/config.py b/lpcraft/config.py
index 2d4cd2a..cdfda54 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -1,6 +1,7 @@
 # Copyright 2021 Canonical Ltd.  This software is licensed under the
 # GNU General Public License version 3 (see the file LICENSE).
 
+import re
 from datetime import timedelta
 from enum import Enum
 from pathlib import Path
@@ -12,6 +13,17 @@ from pydantic import StrictStr
 from lpcraft.utils import load_yaml
 
 
+class _Identifier(pydantic.ConstrainedStr):
+    """A string with constrained syntax used as a short identifier.
+
+    Compare `lp.app.validators.name` in Launchpad, though we also permit
+    underscores here.
+    """
+
+    strict = True
+    regex = re.compile(r"^[a-z0-9][a-z0-9\+\._\-]+$")
+
+
 class ModelConfigDefaults(
     pydantic.BaseModel,
     extra=pydantic.Extra.forbid,
@@ -48,8 +60,8 @@ class Output(ModelConfigDefaults):
 class Job(ModelConfigDefaults):
     """A job definition."""
 
-    series: StrictStr
-    architectures: List[StrictStr]
+    series: _Identifier
+    architectures: List[_Identifier]
     run: Optional[StrictStr]
     environment: Optional[Dict[str, Optional[str]]]
     output: Optional[Output]
@@ -58,8 +70,8 @@ class Job(ModelConfigDefaults):
 
     @pydantic.validator("architectures", pre=True)
     def validate_architectures(
-        cls, v: Union[StrictStr, List[StrictStr]]
-    ) -> List[StrictStr]:
+        cls, v: Union[_Identifier, List[_Identifier]]
+    ) -> List[_Identifier]:
         if isinstance(v, str):
             v = [v]
         return v
@@ -84,7 +96,7 @@ def _expand_job_values(
 class Config(ModelConfigDefaults):
     """A .launchpad.yaml configuration file."""
 
-    pipeline: List[StrictStr]
+    pipeline: List[_Identifier]
     jobs: Dict[StrictStr, List[Job]]
 
     # XXX cjwatson 2021-11-17: This expansion strategy works, but it may
diff --git a/lpcraft/tests/test_config.py b/lpcraft/tests/test_config.py
index e71e9e8..8280223 100644
--- a/lpcraft/tests/test_config.py
+++ b/lpcraft/tests/test_config.py
@@ -84,6 +84,63 @@ class TestConfig(TestCase):
         config = Config.load(path)
         self.assertEqual(["amd64"], config.jobs["test"][0].architectures)
 
+    def test_bad_job_name(self):
+        # Job names must be identifiers.
+        path = self.create_config(
+            dedent(
+                """
+                pipeline:
+                    - foo:bar
+
+                jobs:
+                    'foo:bar':
+                        series: focal
+                        architectures: amd64
+                """
+            )
+        )
+        self.assertRaisesRegex(
+            ValidationError, r"string does not match regex", Config.load, path
+        )
+
+    def test_bad_series_name(self):
+        # Series names must be identifiers.
+        path = self.create_config(
+            dedent(
+                """
+                pipeline:
+                    - test
+
+                jobs:
+                    test:
+                        series: something/bad
+                        architectures: amd64
+                """
+            )
+        )
+        self.assertRaisesRegex(
+            ValidationError, r"string does not match regex", Config.load, path
+        )
+
+    def test_bad_architecture_name(self):
+        # Architecture names must be identifiers.
+        path = self.create_config(
+            dedent(
+                """
+                pipeline:
+                    - test
+
+                jobs:
+                    test:
+                        series: focal
+                        architectures: 'not this'
+                """
+            )
+        )
+        self.assertRaisesRegex(
+            ValidationError, r"string does not match regex", Config.load, path
+        )
+
     def test_expands_matrix(self):
         path = self.create_config(
             dedent(