Add uniqueness for Template Units id (BugFix)#951
Merged
Conversation
A Template unit always contains an id field. It should therefore inherit UnitWithId instead of Unit, so that the `partial_id()` and `id()` methods are inherited as well.
By default, templates using Jinja2 engine gets their id rendered as soon
as they get accessed, but since they don't have the appropriate
environment, the parameters are replaced with an empty string.
This is why when you run the command `checkbox-cli list template`, you
may see things like this:
template 'com.canonical.certification::camera/still_'
instead of
template 'com.canonical.certification::camera/still_{{ name }}'
Note that this does not happen with standard Python formatted strings:
template 'com.canonical.certification::camera/led_{name}'
In order to make the behavior consistent regardless of the template
engine used, this commit:
- introduces a unit property that always returns "template" (similar to
how the Job unit always returns "job").
- modifies Unit.get_record_value() to prevent rendering using Jinja2
templating if the unit is a Checkbox Template
It also contains unit tests for both the TemplateUnit and its Validator.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
==========================================
+ Coverage 38.98% 39.04% +0.06%
==========================================
Files 315 315
Lines 34893 34913 +20
Branches 5971 5976 +5
==========================================
+ Hits 13604 13633 +29
+ Misses 20678 20666 -12
- Partials 611 614 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pieqq
changed the title
Collaborator
Author
|
Discussion with Maciek:
|
PlainBoxObject is used when exposing Units at a high level; for instance, when running `checkbox-cli list template`. Instead of exposing the Template Unit's id field, expose the template-id field.
Collaborator
Author
|
Still need to
|
In order to have more granularity on what is displayed for the template id, use methods similar to the Job Unit.
This method overrides UnitWithId.explain() which displays the Unit's id. In the case of a Template, this is misleading, and the template_id (or, in this case, template_partial_id to prevent displaying the provider namespace) should be used instead. This is useful when reporting errors using `./manage.py validate`, for instance.
Hook25
approved these changes
Jan 29, 2024
Collaborator
Hook25
left a comment
Hook25
left a comment
There was a problem hiding this comment.
+2, 1 for the feature and 1 for the docs update!
LiaoU3
pushed a commit
to LiaoU3/checkbox
that referenced
this pull request
Mar 20, 2024
* Make sure Template units are units with id
A Template unit always contains an id field. It should therefore inherit
UnitWithId instead of Unit, so that the `partial_id()` and `id()`
methods are inherited as well.
* Prevent Template Units id from being rendered when using Jinja2
By default, templates using Jinja2 engine gets their id rendered as soon
as they get accessed, but since they don't have the appropriate
environment, the parameters are replaced with an empty string.
This is why when you run the command `checkbox-cli list template`, you
may see things like this:
template 'com.canonical.certification::camera/still_'
instead of
template 'com.canonical.certification::camera/still_{{ name }}'
Note that this does not happen with standard Python formatted strings:
template 'com.canonical.certification::camera/led_{name}'
In order to make the behavior consistent regardless of the template
engine used, this commit:
- introduces a unit property that always returns "template" (similar to
how the Job unit always returns "job").
- modifies Unit.get_record_value() to prevent rendering using Jinja2
templating if the unit is a Checkbox Template
It also contains unit tests for both the TemplateUnit and its Validator.
* Have Template Unit's "template-id" field generated from the "id" field, if absent
* Expose Template Unit's template-id field instead of id
PlainBoxObject is used when exposing Units at a high level; for
instance, when running `checkbox-cli list template`.
Instead of exposing the Template Unit's id field, expose the template-id
field.
* Use Template partial_id and id, similar to Job's partial_id and id
In order to have more granularity on what is displayed for the template
id, use methods similar to the Job Unit.
* Add unit test to ensure templates have a unique id
* Implement explain() in TemplateUnit
This method overrides UnitWithId.explain() which displays the Unit's id.
In the case of a Template, this is misleading, and the template_id (or,
in this case, template_partial_id to prevent displaying the provider
namespace) should be used instead.
This is useful when reporting errors using `./manage.py validate`, for
instance.
* Update Template Unit documentation about template-id field
* Cleanup Template Unit reference documentation
binli
pushed a commit
to binli/checkbox
that referenced
this pull request
Mar 22, 2024
* Make sure Template units are units with id
A Template unit always contains an id field. It should therefore inherit
UnitWithId instead of Unit, so that the `partial_id()` and `id()`
methods are inherited as well.
* Prevent Template Units id from being rendered when using Jinja2
By default, templates using Jinja2 engine gets their id rendered as soon
as they get accessed, but since they don't have the appropriate
environment, the parameters are replaced with an empty string.
This is why when you run the command `checkbox-cli list template`, you
may see things like this:
template 'com.canonical.certification::camera/still_'
instead of
template 'com.canonical.certification::camera/still_{{ name }}'
Note that this does not happen with standard Python formatted strings:
template 'com.canonical.certification::camera/led_{name}'
In order to make the behavior consistent regardless of the template
engine used, this commit:
- introduces a unit property that always returns "template" (similar to
how the Job unit always returns "job").
- modifies Unit.get_record_value() to prevent rendering using Jinja2
templating if the unit is a Checkbox Template
It also contains unit tests for both the TemplateUnit and its Validator.
* Have Template Unit's "template-id" field generated from the "id" field, if absent
* Expose Template Unit's template-id field instead of id
PlainBoxObject is used when exposing Units at a high level; for
instance, when running `checkbox-cli list template`.
Instead of exposing the Template Unit's id field, expose the template-id
field.
* Use Template partial_id and id, similar to Job's partial_id and id
In order to have more granularity on what is displayed for the template
id, use methods similar to the Job Unit.
* Add unit test to ensure templates have a unique id
* Implement explain() in TemplateUnit
This method overrides UnitWithId.explain() which displays the Unit's id.
In the case of a Template, this is misleading, and the template_id (or,
in this case, template_partial_id to prevent displaying the provider
namespace) should be used instead.
This is useful when reporting errors using `./manage.py validate`, for
instance.
* Update Template Unit documentation about template-id field
* Cleanup Template Unit reference documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
So far, Template Units did not have any unique identifier. The
idfield was used, but it was not unique. This make it difficult to track the origin of an instantiated job.A few things are addressed:
TemplateUnitnow inheritsUnitWithIdinstead ofUnit, which brings, on top of the usualiddefinition, some validators, including that there are no two templates with the sameid.template-idfield is introduced. It is optional: if it's not in the unit definition, it will be computed based on the value of theidfield (using the same string, with characters like{,}andid, it should never be parsed. Before this PR, if the Template unit was using the Jinja2 engine, itsidwould be "rendered", which exposed erroneous information, as explained in commit 81c9535.template-id, an error message will point to the right Template when running./manage.py validate(before, it would point to the unit'sidfield; it now points to thetemplate-idfield)Resolved issues
Required step for CHECKBOX-1074
Documentation
The Template Unit reference documenation has been updated to mention the
template-idfield.Tests
If a provider has 2 different templates defined with the same
id(as it's the case with the base provider before #949) , its validation will now fail:If a provider has 2 different templates that have the same
template-id(or get the same generatedtemplate-idfrom theiridfield), its validation will now fail:(Note that the
fieldmentioned is different from the previous output)As for exposing the template
id:Before:
Vs. After:
Unit tests have been added as well.