launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28026
[Merge] ~cjwatson/launchpad:tighten-lpcraft-parsing into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:tighten-lpcraft-parsing into launchpad:master with ~cjwatson/launchpad:fix-lpcraft-load-interface as a prerequisite.
Commit message:
Tighten up lpcraft parsing
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414688
This now matches the relevant parts of https://git.launchpad.net/lpcraft/tree/lpcraft/config.py a little more closely: the top-level "pipeline" and "jobs" keys are now required, as are the "series" and "architectures" keys of each job definition, which between them are the only things Launchpad will need to look at. Each element of the "pipeline" key is now normalized into a list.
The returned configuration object also now implements a minimal `ILPCraftConfiguration` interface, making it easier to handle cases where it's returned from a security-proxied method and thus ends up being security-proxied itself.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:tighten-lpcraft-parsing into launchpad:master.
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index 8ba4643..76c6986 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -1274,6 +1274,11 @@
<webservice:register module="lp.code.interfaces.webservice" />
+ <!-- LPCraftConfiguration -->
+ <class class="lp.code.model.lpcraft.LPCraftConfiguration">
+ <allow interface="lp.code.interfaces.lpcraft.ILPCraftConfiguration" />
+ </class>
+
<adapter
factory="lp.code.browser.sourcepackagerecipe.distroseries_renderer"
name="distroseries"/>
diff --git a/lib/lp/code/interfaces/lpcraft.py b/lib/lp/code/interfaces/lpcraft.py
new file mode 100644
index 0000000..30c8b67
--- /dev/null
+++ b/lib/lp/code/interfaces/lpcraft.py
@@ -0,0 +1,34 @@
+# Copyright 2022 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interface to lpcraft's configuration file."""
+
+__all__ = [
+ "ILPCraftConfiguration",
+ "LPCraftConfigurationError",
+ ]
+
+from zope.interface import Interface
+from zope.schema import (
+ Dict,
+ List,
+ TextLine,
+ )
+
+
+class LPCraftConfigurationError(Exception):
+ """Parsing lpcraft's configuration file failed."""
+
+
+class ILPCraftConfiguration(Interface):
+ """An object representation of a `.launchpad.yaml` file."""
+
+ pipeline = List(
+ title="List of stages",
+ description="Each stage is a list of job names.",
+ value_type=TextLine())
+
+ jobs = Dict(
+ title="Mapping of job names to job definitions",
+ key_type=TextLine(),
+ value_type=Dict(key_type=TextLine()))
diff --git a/lib/lp/code/model/lpcraft.py b/lib/lp/code/model/lpcraft.py
index 02124f9..4f932f4 100644
--- a/lib/lp/code/model/lpcraft.py
+++ b/lib/lp/code/model/lpcraft.py
@@ -16,6 +16,12 @@ we are on Python 3.8
__all__ = ["load_configuration"]
import yaml
+from zope.interface import implementer
+
+from lp.code.interfaces.lpcraft import (
+ ILPCraftConfiguration,
+ LPCraftConfigurationError,
+ )
def _expand_job_values(values):
@@ -44,18 +50,37 @@ def load_configuration(configuration_file):
"""
# load yaml
content = yaml.safe_load(configuration_file)
- # expand matrix
+ if content is None:
+ raise LPCraftConfigurationError("Empty configuration file")
+ for required_key in "pipeline", "jobs":
+ if required_key not in content:
+ raise LPCraftConfigurationError(
+ "Configuration file does not declare '{}'".format(
+ required_key))
+ # normalize each element of `pipeline` into a list
expanded_values = content.copy()
- if expanded_values.get("jobs"):
- expanded_values["jobs"] = {
- job_name: _expand_job_values(job_values)
- for job_name, job_values in content["jobs"].items()
- }
+ expanded_values["pipeline"] = [
+ [stage] if isinstance(stage, str) else stage
+ for stage in expanded_values["pipeline"]]
+ # expand matrix
+ expanded_values["jobs"] = {
+ job_name: _expand_job_values(job_values)
+ for job_name, job_values in content["jobs"].items()
+ }
+ for job_name, expanded_job_values in expanded_values["jobs"].items():
+ for i, job_values in enumerate(expanded_job_values):
+ for required_key in "series", "architectures":
+ if required_key not in job_values:
+ raise LPCraftConfigurationError(
+ "Job {}:{} does not declare '{}'".format(
+ job_name, i, required_key))
# create "data class"
- return Configuration(expanded_values)
+ return LPCraftConfiguration(expanded_values)
+
+@implementer(ILPCraftConfiguration)
+class LPCraftConfiguration:
+ """See `ILPCraftConfiguration`."""
-class Configuration:
- """configuration object representation of a `.launchpad.yaml` file"""
def __init__(self, d):
self.__dict__.update(d)
diff --git a/lib/lp/code/model/tests/test_lpcraft.py b/lib/lp/code/model/tests/test_lpcraft.py
index 7e29755..3b6031d 100644
--- a/lib/lp/code/model/tests/test_lpcraft.py
+++ b/lib/lp/code/model/tests/test_lpcraft.py
@@ -3,50 +3,132 @@
from textwrap import dedent
+from lp.code.interfaces.lpcraft import (
+ ILPCraftConfiguration,
+ LPCraftConfigurationError,
+ )
from lp.code.model.lpcraft import load_configuration
from lp.testing import TestCase
class TestLoadConfiguration(TestCase):
- def test_load_configuration_with_pipeline(self):
+ def test_configuration_implements_interface(self):
c = dedent("""\
pipeline:
- test
+ jobs:
+ test:
+ series: focal
+ architectures: [amd64]
""")
configuration = load_configuration(c)
- self.assertEqual(
- ["test"], configuration.pipeline
- )
+ self.assertProvides(configuration, ILPCraftConfiguration)
- def test_load_configuration_with_pipeline_and_jobs(self):
+ def test_load_configuration_empty(self):
+ self.assertRaisesWithContent(
+ LPCraftConfigurationError, "Empty configuration file",
+ load_configuration, "")
+
+ def test_load_configuration_with_no_pipeline(self):
+ c = dedent("""\
+ jobs:
+ test: {}
+ """)
+
+ self.assertRaisesWithContent(
+ LPCraftConfigurationError,
+ "Configuration file does not declare 'pipeline'",
+ load_configuration, c)
+
+ def test_load_configuration_with_pipeline_but_no_jobs(self):
+ c = dedent("""\
+ pipeline:
+ - test
+ """)
+
+ self.assertRaisesWithContent(
+ LPCraftConfigurationError,
+ "Configuration file does not declare 'jobs'",
+ load_configuration, c)
+
+ def test_load_configuration_with_job_with_no_series(self):
+ c = dedent("""\
+ pipeline:
+ - test
+ jobs:
+ test: {}
+ """)
+
+ self.assertRaisesWithContent(
+ LPCraftConfigurationError,
+ "Job test:0 does not declare 'series'",
+ load_configuration, c)
+
+ def test_load_configuration_with_job_with_no_architectures(self):
c = dedent("""\
pipeline:
- test
jobs:
test:
- series:
- focal
+ series: focal
+ """)
+
+ self.assertRaisesWithContent(
+ LPCraftConfigurationError,
+ "Job test:0 does not declare 'architectures'",
+ load_configuration, c)
+
+ def test_load_configuration_with_pipeline_and_jobs(self):
+ c = dedent("""\
+ pipeline:
+ - [test, lint]
+ - [publish]
+ jobs:
+ test:
+ series: focal
+ architectures: [amd64]
lint:
- series:
- focal
+ series: focal
+ architectures: [amd64]
publish:
- series:
- focal
+ series: focal
+ architectures: [amd64]
""")
configuration = load_configuration(c)
self.assertEqual(
- ["test"], configuration.pipeline,
+ [["test", "lint"], ["publish"]], configuration.pipeline,
+ )
+ self.assertEqual(
+ {
+ "test": [{'series': 'focal', 'architectures': ['amd64']}],
+ "lint": [{'series': 'focal', 'architectures': ['amd64']}],
+ "publish": [{'series': 'focal', 'architectures': ['amd64']}],
+ }, configuration.jobs,
)
+ def test_expand_pipeline(self):
+ # if `pipeline` is a string, it will be converted into a list
+ c = dedent("""\
+ pipeline:
+ - test
+ jobs:
+ test:
+ series: focal
+ architectures: [amd64]
+ """)
+
+ configuration = load_configuration(c)
+
+ self.assertEqual(
+ [["test"]], configuration.pipeline,
+ )
self.assertEqual(
{
- "test": [{'series': 'focal'}],
- "lint": [{'series': 'focal'}],
- "publish": [{'series': 'focal'}],
+ "test": [{'series': 'focal', 'architectures': ['amd64']}],
}, configuration.jobs,
)
@@ -54,7 +136,7 @@ class TestLoadConfiguration(TestCase):
# if `architectures` is a string, it will be converted into a list
c = dedent("""\
pipeline:
- - test
+ - [test]
jobs:
test:
matrix:
@@ -67,9 +149,8 @@ class TestLoadConfiguration(TestCase):
configuration = load_configuration(c)
self.assertEqual(
- ["test"], configuration.pipeline,
+ [["test"]], configuration.pipeline,
)
-
self.assertEqual(
{
'test': [
