Merge ~sylvain-pineau/checkbox-ng:fix-1748371 into checkbox-ng:master

Proposed by Sylvain Pineau
Status: Merged
Approved by: Sylvain Pineau
Approved revision: 289c513e8a4e8cf29bd1fe2d5b649c80fc117130
Merged at revision: 7275e5675311b624e5911b374cc187379995f88d
Proposed branch: ~sylvain-pineau/checkbox-ng:fix-1748371
Merge into: checkbox-ng:master
Diff against target: 147 lines (+29/-2)
3 files modified
docs/units/template.rst (+8/-2)
plainbox/impl/session/assistant.py (+3/-0)
plainbox/impl/unit/unit.py (+18/-0)
Reviewer Review Type Date Requested Status
Maciej Kisielewski Approve
Sylvain Pineau (community) Needs Resubmitting
Review via email: mp+337988@code.launchpad.net

Description of the change

New parameter for jinja2 templated jobs (Nota: not only template units), the content of the checkbox config environment section.

Fixes the linked bug, to avoid job description with for example hardcoded <ap_name> where in config we set $AP_NAME.

To post a comment you must log in.
Revision history for this message
Maciej Kisielewski (kissiel) wrote :

I'm going to call for a better solution for the problem.
I really don't like passing the config through provider units.
Runtime configuration should not be a provider's property (logically it isn't). I understand the need for environment info to be present when instantiating templates, but I think we don't need to get providers involved in this dynamic process (providers are fairly static monsters).
What I would advocate doing instead is:
 - pass config to the code that does the instantiation (IDK how many steps in the callstack the config would have to be passed). or,
 - exposing the current config (or current SA instance) through a module interface, I'm doing something similar for the remote functionality, see remote.py: 75-87

review: Needs Fixing
Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

New version using a class attribute to store the config rather than passing through provider objects. should be better in term of memory consumption IMHO.

review: Needs Resubmitting
Revision history for this message
Maciej Kisielewski (kissiel) wrote :

Great stuff, thanks! +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/docs/units/template.rst b/docs/units/template.rst
2index f16e314..5b31010 100644
3--- a/docs/units/template.rst
4+++ b/docs/units/template.rst
5@@ -176,7 +176,13 @@ Plainbox populates the template parameter dictionary with some extra keys to aid
6 ``__index__``:
7 If a template unit can result in N content jobs then this variable is equal to how many jobs have been created so far.
8
9+Following parameters are only available for ``template-engine``: ``jinja2``:
10+
11 ``__system_env__``:
12- When the plainbox encounters a template to render it will populate this variable with the executing shell's enviroment variables as ``os.environ``
13+ When checkbox encounters a template to render it will populate this variable with the executing shell's enviroment variables as ``os.environ``
14+
15+``__on_ubuntucore__``:
16+ Helper function (boolean) checking if checkbox runs from on ubuntu core
17
18- Available for ``template-engine``: ``jinja2``
19+``__checkbox_env__``:
20+ Dictionary containing the checkbox config environment section
21diff --git a/plainbox/impl/session/assistant.py b/plainbox/impl/session/assistant.py
22index db60157..4836c8c 100644
23--- a/plainbox/impl/session/assistant.py
24+++ b/plainbox/impl/session/assistant.py
25@@ -61,6 +61,7 @@ from plainbox.impl.session.restart import detect_restart_strategy
26 from plainbox.impl.session.storage import SessionStorageRepository
27 from plainbox.impl.transport import OAuthTransport
28 from plainbox.impl.transport import TransportError
29+from plainbox.impl.unit.unit import Unit
30 from plainbox.vendor import morris
31
32 _logger = logging.getLogger("plainbox.session.assistant")
33@@ -168,6 +169,7 @@ class SessionAssistant:
34 self._api_flags = api_flags
35 self._repo = SessionStorageRepository()
36 self._config = PlainBoxConfig().get()
37+ Unit.config = self._config
38 self._execution_ctrl_list = None # None is "default"
39 self._ctrl_setup_list = []
40 # List of providers that were selected. This is buffered until a
41@@ -335,6 +337,7 @@ class SessionAssistant:
42 """
43 UsageExpectation.of(self).enforce()
44 self._config = config
45+ Unit.config = config
46 # NOTE: We expect applications to call this at most once.
47 del UsageExpectation.of(self).allowed_calls[
48 self.use_alternate_configuration]
49diff --git a/plainbox/impl/unit/unit.py b/plainbox/impl/unit/unit.py
50index ec1cd55..cd7b365 100644
51--- a/plainbox/impl/unit/unit.py
52+++ b/plainbox/impl/unit/unit.py
53@@ -36,6 +36,7 @@ from jinja2 import Template
54 from plainbox.i18n import gettext as _
55 from plainbox.impl.decorators import cached_property
56 from plainbox.impl.decorators import instance_method_lru_cache
57+from plainbox.impl.secure.config import Unset
58 from plainbox.impl.secure.origin import Origin
59 from plainbox.impl.secure.rfc822 import normalize_rfc822_value
60 from plainbox.impl.symbol import Symbol
61@@ -380,6 +381,8 @@ class Unit(metaclass=UnitType):
62 class.
63 """
64
65+ config = None
66+
67 def __init__(self, data, raw_data=None, origin=None, provider=None,
68 parameters=None, field_offset_map=None, virtual=False):
69 """
70@@ -605,6 +608,13 @@ class Unit(metaclass=UnitType):
71 field_offset_map=record.field_offset_map)
72
73 @instance_method_lru_cache(maxsize=None)
74+ def _checkbox_env(self):
75+ if self.config is not None and self.config.environment is not Unset:
76+ return self.config.environment
77+ else:
78+ return {}
79+
80+ @instance_method_lru_cache(maxsize=None)
81 def get_record_value(self, name, default=None):
82 """
83 Obtain the normalized value of the specified record attribute
84@@ -628,6 +638,7 @@ class Unit(metaclass=UnitType):
85 # template instantiation we avoid problems with creation of
86 # checkpoints
87 tmp_params = self.parameters.copy()
88+ tmp_params.update({'__checkbox_env__': self._checkbox_env()})
89 tmp_params.update({'__system_env__': os.environ})
90 tmp_params.update({'__on_ubuntucore__': on_ubuntucore()})
91 value = Template(value).render(tmp_params)
92@@ -640,6 +651,7 @@ class Unit(metaclass=UnitType):
93 elif (value is not None and self.template_engine == 'jinja2'
94 and not self.is_parametric):
95 tmp_params = {
96+ '__checkbox_env__': self._checkbox_env(),
97 '__system_env__': os.environ,
98 '__on_ubuntucore__': on_ubuntucore()
99 }
100@@ -669,6 +681,7 @@ class Unit(metaclass=UnitType):
101 if value is not None and self.is_parametric:
102 if self.template_engine == 'jinja2':
103 tmp_params = self.parameters.copy()
104+ tmp_params.update({'__checkbox_env__': self._checkbox_env()})
105 tmp_params.update({'__system_env__': os.environ})
106 tmp_params.update({'__on_ubuntucore__': on_ubuntucore()})
107 value = Template(value).render(tmp_params)
108@@ -677,6 +690,7 @@ class Unit(metaclass=UnitType):
109 elif (value is not None and self.template_engine == 'jinja2'
110 and not self.is_parametric):
111 tmp_params = {
112+ '__checkbox_env__': self._checkbox_env(),
113 '__system_env__': os.environ,
114 '__on_ubuntucore__': on_ubuntucore()
115 }
116@@ -722,6 +736,7 @@ class Unit(metaclass=UnitType):
117 # of the problem?
118 if self.template_engine == 'jinja2':
119 tmp_params = self.parameters.copy()
120+ tmp_params.update({'__checkbox_env__': self._checkbox_env()})
121 tmp_params.update({'__system_env__': os.environ})
122 tmp_params.update({'__on_ubuntucore__': on_ubuntucore()})
123 msgstr = Template(msgstr).render(tmp_params)
124@@ -730,6 +745,7 @@ class Unit(metaclass=UnitType):
125 msgstr, (), self.parameters)
126 elif self.template_engine == 'jinja2':
127 tmp_params = {
128+ '__checkbox_env__': self._checkbox_env(),
129 '__system_env__': os.environ,
130 '__on_ubuntucore__': on_ubuntucore()
131 }
132@@ -744,6 +760,7 @@ class Unit(metaclass=UnitType):
133 if self.is_parametric:
134 if self.template_engine == 'jinja2':
135 tmp_params = self.parameters.copy()
136+ tmp_params.update({'__checkbox_env__': self._checkbox_env()})
137 tmp_params.update({'__system_env__': os.environ})
138 tmp_params.update({'__on_ubuntucore__': on_ubuntucore()})
139 msgstr = Template(msgstr).render(tmp_params)
140@@ -752,6 +769,7 @@ class Unit(metaclass=UnitType):
141 msgstr, (), self.parameters)
142 elif self.template_engine == 'jinja2':
143 tmp_params = {
144+ '__checkbox_env__': self._checkbox_env(),
145 '__system_env__': os.environ,
146 '__on_ubuntucore__': on_ubuntucore()
147 }

Subscribers

People subscribed via source and target branches