diff --git a/checkbox-ng/plainbox/impl/highlevel.py b/checkbox-ng/plainbox/impl/highlevel.py index 6d5f8000b..8991b2bf3 100644 --- a/checkbox-ng/plainbox/impl/highlevel.py +++ b/checkbox-ng/plainbox/impl/highlevel.py @@ -293,9 +293,11 @@ def _file_to_obj(self, unit): def _template_to_obj(self, unit): return PlainBoxObject( - unit, group=unit.Meta.name, name=unit.id, attrs=OrderedDict(( + unit, group=unit.Meta.name, name=unit.template_id, + attrs=OrderedDict(( ('id', unit.id), ('partial_id', unit.partial_id), + ('template_id', unit.template_id), ('template_unit', unit.template_unit), ('template_resource', unit.template_resource), ('template_filter', unit.template_filter), diff --git a/checkbox-ng/plainbox/impl/test_highlevel.py b/checkbox-ng/plainbox/impl/test_highlevel.py new file mode 100644 index 000000000..619272d9c --- /dev/null +++ b/checkbox-ng/plainbox/impl/test_highlevel.py @@ -0,0 +1,40 @@ +# This file is part of Checkbox. +# +# Copyright 2024 Canonical Ltd. +# Written by: +# Pierre Equoy +# +# Checkbox is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 3, +# as published by the Free Software Foundation. +# +# Checkbox 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 Checkbox. If not, see . + +from unittest import TestCase + +from plainbox.impl.highlevel import Explorer +from plainbox.impl.unit.template import TemplateUnit + + +class TestExplorer(TestCase): + def test_template_to_obj__without_template_id(self): + template = TemplateUnit({ + "id": "id", + }) + explorer = Explorer() + obj = explorer._template_to_obj(template) + self.assertEqual(obj.name, "id") + + def test_template_to_obj__with_template_id(self): + template = TemplateUnit({ + "template-id": "template-id", + }) + explorer = Explorer() + obj = explorer._template_to_obj(template) + self.assertEqual(obj.name, "template-id") diff --git a/checkbox-ng/plainbox/impl/unit/template.py b/checkbox-ng/plainbox/impl/unit/template.py index c78119dd0..3df94fc07 100644 --- a/checkbox-ng/plainbox/impl/unit/template.py +++ b/checkbox-ng/plainbox/impl/unit/template.py @@ -23,6 +23,7 @@ """ import itertools import logging +import string from plainbox.i18n import gettext as _ from plainbox.i18n import gettext_noop as N_ @@ -35,13 +36,13 @@ from plainbox.impl.unit import all_units from plainbox.impl.unit import concrete_validators from plainbox.impl.unit import get_accessed_parameters -from plainbox.impl.unit.unit import Unit -from plainbox.impl.unit.unit import UnitValidator +from plainbox.impl.unit.unit_with_id import UnitWithId +from plainbox.impl.unit.unit_with_id import UnitWithIdValidator from plainbox.impl.unit.validators import CorrectFieldValueValidator from plainbox.impl.unit.validators import ReferenceConstraint from plainbox.impl.unit.validators import UnitReferenceValidator +from plainbox.impl.unit.validators import UniqueValueValidator from plainbox.impl.validation import Problem -from plainbox.impl.validation import Severity __all__ = ['TemplateUnit'] @@ -50,7 +51,7 @@ logger = logging.getLogger("plainbox.unit.template") -class TemplateUnitValidator(UnitValidator): +class TemplateUnitValidator(UnitWithIdValidator): """Validator for template unit.""" @@ -77,8 +78,28 @@ def check(self, unit): self.issue_list.append(issue) yield issue + def explain(self, unit, field, kind, message): + """ + Lookup an explanatory string for a given issue kind + + :returns: + A string (explanation) or None if the issue kind + is not known to this method. + + This version overrides the base implementation to use the unit + template_id, if it is available, when reporting issues. + """ + if unit.template_partial_id is None: + return super().explain(unit, field, kind, message) + stock_msg = self._explain_map.get(kind) + if stock_msg is None: + return None + return _("{unit} {id!a}, field {field!a}, {message}").format( + unit=unit.tr_unit(), id=unit.template_partial_id, field=str(field), + message=message or stock_msg) + -class TemplateUnit(Unit): +class TemplateUnit(UnitWithId): """ Template that can instantiate zero or more additional units. @@ -166,21 +187,13 @@ def __str__(self): return "{} <~ {}".format(self.id, self.resource_id) @property - def partial_id(self): + def unit(self): """ - Identifier of this job, without the provider name. + The value of the unit field (overridden) - This field should not be used anymore, except for display + The return value is always "template" """ - return self.get_record_value('id', '?') - - @property - def id(self): - """Identifier of this template unit.""" - if self.provider: - return "{}::{}".format(self.provider.namespace, self.partial_id) - else: - return self.partial_id + return "template" @property def resource_partial_id(self): @@ -216,6 +229,44 @@ def resource_id(self): else: return "{}::{}".format(resource_namespace, resource_partial_id) + @classmethod + def slugify_template_id(cls, _string=None): + """ + Remove unwanted characters from a raw job id string. + + This helps exposing cleaner looking template ids when the id is + generated from the id field by removing characters like '{', '}', + and ' '. + """ + if _string: + valid_chars = frozenset( + "-_.:/\\{}{}".format(string.ascii_letters, string.digits) + ) + return "".join(c if c in valid_chars else "" for c in _string) + + @property + def template_partial_id(self): + """ + Identifier of this template, without the provider namespace. + + If the ``template-id`` field is not present in the unit definition, + ``template_partial_id`` is computed from the ``partial_id`` attribute. + """ + template_partial_id = self.get_record_value("template-id") + if not template_partial_id: + template_partial_id = self.slugify_template_id(self.partial_id) + return template_partial_id + + @property + def template_id(self): + """Identifier of this template, with the provider namespace.""" + if self.provider and self.template_partial_id: + return "{}::{}".format(self.provider.namespace, + self.template_partial_id + ) + else: + return self.template_partial_id + @property def template_resource(self): """value of the 'template-resource' field.""" @@ -436,6 +487,7 @@ class fields(SymbolDef): """Symbols for each field that a TemplateUnit can have.""" + template_id = "template-id" template_unit = 'template-unit' template_resource = 'template-resource' template_filter = 'template-filter' @@ -444,6 +496,17 @@ class fields(SymbolDef): validator_cls = TemplateUnitValidator field_validators = { + fields.template_id: [ + concrete_validators.untranslatable, + concrete_validators.templateVariant, + UniqueValueValidator(), + # We want to have bare, namespace-less identifiers + CorrectFieldValueValidator( + lambda value, unit: ( + "::" not in unit.get_record_value("template-id")), + message=_("identifier cannot define a custom namespace"), + onlyif=lambda unit: unit.get_record_value("template-id")), + ], fields.template_unit: [ concrete_validators.untranslatable, ], diff --git a/checkbox-ng/plainbox/impl/unit/test_template.py b/checkbox-ng/plainbox/impl/unit/test_template.py index 97216f1b6..180ddb270 100644 --- a/checkbox-ng/plainbox/impl/unit/test_template.py +++ b/checkbox-ng/plainbox/impl/unit/test_template.py @@ -42,6 +42,14 @@ class TemplateUnitTests(TestCase): + def test_id(self): + template = TemplateUnit({ + "template-resource": "resource", + "template-id": "check-devices", + "id": "check-device-{dev_name}", + }) + self.assertEqual(template.id, "check-device-{dev_name}") + def test_resource_partial_id__empty(self): """ Ensure that ``resource_partial_id`` defaults to None @@ -167,6 +175,54 @@ def test_resource_id__template_and_provider_ns(self): 'template-resource': 'rc' }, provider=provider).resource_id, 'namespace::rc') + def test_slugify(self): + self.assertEqual( + TemplateUnit.slugify_template_id("stress/benchmark_{disk}"), + "stress/benchmark_disk" + ) + self.assertEqual( + TemplateUnit.slugify_template_id("ns::stress/benchmark_{disk}"), + "ns::stress/benchmark_disk" + ) + self.assertEqual( + TemplateUnit.slugify_template_id("suspend_{{ iterations }}_times"), + "suspend_iterations_times" + ) + self.assertEqual(TemplateUnit.slugify_template_id(), None) + + def test_template_id(self): + self.assertEqual(TemplateUnit({ + "template-id": "template_id", + }).template_id, "template_id") + + def test_template_id__from_job_id(self): + self.assertEqual(TemplateUnit({ + "id": "job_id_{param}", + }).template_id, "job_id_param") + + def test_template_id__precedence(self): + """Ensure template-id takes precedence over job id.""" + self.assertEqual(TemplateUnit({ + "template-id": "template_id", + "id": "job_id_{param}", + }).template_id, "template_id") + + def test_template_id__from_job_id_jinja2(self): + self.assertEqual(TemplateUnit({ + "template-resource": "resource", + "template-engine": "jinja2", + "id": "job_id_{{ param }}", + }).template_id, "job_id_param") + + def test_template_id__precedence_jinja2(self): + """Ensure template-id takes precedence over Jinja2-templated job id.""" + self.assertEqual(TemplateUnit({ + "template-id": "template_id", + "template-resource": "resource", + "template-engine": "jinja2", + "id": "job_id_{{ param }}", + }).template_id, "template_id") + def test_template_resource__empty(self): self.assertEqual(TemplateUnit({}).template_resource, None) @@ -351,6 +407,14 @@ def test_instantiate_all(self): class TemplateUnitJinja2Tests(TestCase): + def test_id_jinja2(self): + template = TemplateUnit({ + 'template-resource': 'resource', + 'template-engine': 'jinja2', + 'id': 'check-device-{{ dev_name }}', + }) + self.assertEqual(template.id, "check-device-{{ dev_name }}") + def test_instantiate_one_jinja2(self): template = TemplateUnit({ 'template-resource': 'resource', @@ -374,6 +438,55 @@ class TemplateUnitFieldValidationTests(UnitFieldValidationTests): unit_cls = TemplateUnit + def test_template_id__untranslatable(self): + issue_list = self.unit_cls({ + '_template-id': 'template_id' + }, provider=self.provider).check() + self.assertIssueFound( + issue_list, self.unit_cls.Meta.fields.template_id, + Problem.unexpected_i18n, Severity.warning) + + def test_template_id__bare(self): + issue_list = self.unit_cls({ + "template-id": "ns::id" + }, provider=self.provider).check() + message = ("template 'ns::id', field 'template-id', identifier cannot " + "define a custom namespace") + self.assertIssueFound( + issue_list, self.unit_cls.Meta.fields.template_id, + Problem.wrong, Severity.error, message) + + def test_template_id__unique(self): + unit = self.unit_cls({ + 'template-id': 'id' + }, provider=self.provider) + other_unit = self.unit_cls({ + 'template-id': 'id' + }, provider=self.provider) + self.provider.unit_list = [unit, other_unit] + self.provider.problem_list = [] + context = UnitValidationContext([self.provider]) + message_start = ( + "{} 'id', field 'template-id', clashes with 1 other unit," + " look at: " + ).format(unit.tr_unit()) + issue_list = unit.check(context=context) + issue = self.assertIssueFound( + issue_list, self.unit_cls.Meta.fields.template_id, + Problem.not_unique, Severity.error) + self.assertTrue(issue.message.startswith(message_start)) + + def test_unit__present(self): + """ + TemplateUnit.unit always returns "template", the default error for the + base Unit class should never happen. + """ + issue_list = self.unit_cls({ + }, provider=self.provider).check() + message = "field 'unit', unit should explicitly define its type" + self.assertIssueNotFound(issue_list, self.unit_cls.Meta.fields.unit, + Problem.missing, Severity.advice, message) + def test_template_unit__untranslatable(self): issue_list = self.unit_cls({ # NOTE: the value must be a valid unit! diff --git a/checkbox-ng/plainbox/impl/unit/test_unit.py b/checkbox-ng/plainbox/impl/unit/test_unit.py index bdc231884..06f45d2ce 100644 --- a/checkbox-ng/plainbox/impl/unit/test_unit.py +++ b/checkbox-ng/plainbox/impl/unit/test_unit.py @@ -79,6 +79,49 @@ def assertIssueFound(self, issue_list, field=None, kind=None, '\n'.join(" - {!r}".format(issue) for issue in issue_list)) return self.fail(msg) + def assertIssueNotFound(self, issue_list, field=None, kind=None, + severity=None, message=None): + """ + Raise an assertion if no issue matching the provided criteria is found + + :param issue_list: + A list of issues to look through + :param field: + (optional) value that must match the same attribute on the Issue + :param kind: + (optional) value that must match the same attribute on the Issue + :param severity: + (optional) value that must match the same attribute on the Issue + :param message: + (optional) value that must match the same attribute on the Issue + :returns: + The issue matching those constraints, if found + """ + for issue in issue_list: + if field is not None and issue.field is not field: + continue + if severity is not None and issue.severity is not severity: + continue + if kind is not None and issue.kind is not kind: + continue + if message is not None and issue.message != message: + continue + # return issue + return self.fail("Issue matching the given criteria found!") + msg = "Issue matching:\n{}\nwas found in:\n{}".format( + '\n'.join( + ' * {} is {!r}'.format(issue_attr, value) + for issue_attr, value in + [('field', field), + ('severity', severity), + ('kind', kind), + ('message', message)] + if value is not None), + '\n'.join(" - {!r}".format(issue) for issue in issue_list)) + return self.fail(msg) + else: + return issue + class TestUnitDefinition(TestCase): diff --git a/checkbox-ng/plainbox/impl/unit/unit.py b/checkbox-ng/plainbox/impl/unit/unit.py index 8cc84946f..1e1640636 100644 --- a/checkbox-ng/plainbox/impl/unit/unit.py +++ b/checkbox-ng/plainbox/impl/unit/unit.py @@ -713,6 +713,7 @@ def get_record_value(self, name, default=None): value is not None and self.template_engine == "jinja2" and not self.is_parametric + and not self.unit == "template" ): tmp_params = { "__checkbox_env__": self._checkbox_env(), @@ -755,6 +756,7 @@ def get_raw_record_value(self, name, default=None): value is not None and self.template_engine == "jinja2" and not self.is_parametric + and not self.unit == "template" ): tmp_params = { "__checkbox_env__": self._checkbox_env(), diff --git a/docs/reference/units/template.rst b/docs/reference/units/template.rst index 296ef0c78..eeb013def 100644 --- a/docs/reference/units/template.rst +++ b/docs/reference/units/template.rst @@ -4,21 +4,21 @@ Template Unit ============= -The template unit is a variant of Plainbox unit types. A template is a skeleton +The template unit is a variant of Checkbox unit types. A template is a skeleton for defining additional units, typically job definitions. A template is defined -as a typical RFC822-like Plainbox unit (like a typical job definition) with the +as a typical RFC822-like Checkbox unit (like a typical job definition) with the exception that all the fields starting with the string ``template-`` are reserved for the template itself while all the other fields are a definition of all the eventual instances of the template. -There is one particular requirement on the job's ``id`` field for the instances -to be generated. This ``id`` field value must be template-based, for example :: +There are two requirements for the job's ``id`` field for the instances to +be generated. It must be unique, and it must be template-based, for example:: unit: template template-resource: graphics_card template-engine: jinja2 template-unit: job - id: chromium_webcam_encoding_{{driver}}_{{bus}} + id: chromium_webcam_encoding_{{ driver }}_{{ bus }} instead of:: @@ -29,13 +29,21 @@ instead of:: id: chromium_webcam_encoding Template-Specific Fields ------------------------- +======================== -There are four fields that are specific to the template unit: +.. _Template template-id field: + +``template-id`` + Unique identifier for this template. + + This field is optional. If absent, a ``template-id`` value will be computed + from the ``id`` field. For instance, if the ``id`` field is + ``stress/reboot_{iterations}_times``, the computed ``template-id`` field + will be ``stress/reboot_iterations_times``. .. _Template template-unit field: -``template-unit``: +``template-unit`` Name of the unit type this template will generate. By default job definition units are generated (as if the field was specified with the value of ``job``) eventually but other values may be used as well. @@ -44,7 +52,7 @@ There are four fields that are specific to the template unit: .. _Template template-resource field: -``template-resource``: +``template-resource`` Name of the resource job (if it is a compatible resource identifier) to use to parametrize the template. This must either be a name of a resource job available in the namespace the template unit belongs to *or* a valid @@ -55,7 +63,7 @@ There are four fields that are specific to the template unit: .. _Template template-imports field: -``template-imports``: +``template-imports`` A resource import statement. It can be used to refer to arbitrary resource job by its full identifier and (optionally) give it a short variable name. @@ -75,7 +83,7 @@ There are four fields that are specific to the template unit: .. _Template template-filter field: -``template-filter``: +``template-filter`` A resource program that limits the set of records from which template instances will be made. The syntax of this field is the same as the syntax of typical job definition unit's ``requires`` field, that is, it is a @@ -89,14 +97,14 @@ There are four fields that are specific to the template unit: .. _Template template-engine field: -``template-engine``: +``template-engine`` Name of the template engine to use, default is python string formatting - (See PEP 3101). Currently the only other supported engine is jinja2. + (See PEP 3101). The only other supported engine is ``jinja2``. This field is optional. Instantiation -------------- +============= When a template is instantiated, a single record object is used to fill in the parametric values to all the applicable fields. Each field is formatted using @@ -167,19 +175,24 @@ hard drive available on the system:: block_device.{name}_state != 'removable' user: root command: disk_stats_test {name} - _description: This test checks {name} disk stats, generates some activity and rechecks stats to verify they've changed. It also verifies that disks appear in the various files they're supposed to. + _description: This test checks {name} disk stats, generates some activity + and rechecks stats to verify they've changed. It also verifies that disks + appear in the various files they're supposed to. The ``template-resource`` used here (``device``) refers to a resource job using the ``udev_resource`` script to get information about the system. The ``udev_resource`` script returns a list of items with attributes such as ``path`` and ``name``, so we can use these directly in our template. -``block_device`` is an other resource unit used for setting a requirement on the state of the current device. +``block_device`` is an other resource unit used for setting a requirement +on the state of the current device. Simple Jinja templates example ------------------------------ -Jinja2 can be used as the templating engine instead of python string formatting. This allows the author to access some powerful templating features including expressions. +Jinja2 can be used as the templating engine instead of python string +formatting. This allows the author to access some powerful templating features +including expressions. First here is the previous disk stats example converted to jinja2:: @@ -195,18 +208,22 @@ First here is the previous disk stats example converted to jinja2:: block_device.{{ name }}_state != 'removable' user: root command: disk_stats_test {{ name }} - _description: This test checks {{ name }} disk stats, generates some activity and rechecks stats to verify they've changed. It also verifies that disks appear in the various files they're supposed to. + _description: This test checks {{ name }} disk stats, generates some + activity and rechecks stats to verify they've changed. It also verifies + that disks appear in the various files they're supposed to. Template engine additional features ------------------------------------ +=================================== -Plainbox populates the template parameter dictionary with some extra keys to aid the author. +Checkbox populates the template parameter dictionary with some extra keys +to aid the author. ``__index__``: If a template unit can result in N content jobs then this variable is equal to how many jobs have been created so far. -Following parameters are only available for ``template-engine``: ``jinja2``: +Following parameters are only available for templates based on the Jinja2 +engine (see :ref:`Template template-engine field`): ``__system_env__``: When checkbox encounters a template to render it will populate this