Differential D202

new cli arg - variable value override
ClosedPublic
Actions

Authored by lbrabec on Aug 13 2014, 10:52 AM.

Details

Reviewers
kparal
tflink
jskladan
Maniphest Tasks
T244: Create "export_variable" directive
Commits
rLTRN11434de4f656: new cli arg - variable value override
Summary

easy fix fox T244

variable values can be overridden from CLI arg (-o and --override)

Test Plan

py.test

python runtask.py -i xchat-2.8.8-21.fc20 -t koji_build -a x86_64 ../task-rpmlint/rpmlint.yml -o "workdir='/some/dir/' arch=['i386']"
values must be 'eval() acceptable' (string, list of strings..)

Diff Detail

Repository
rLTRN libtaskotron
Branch
feature/T244
Lint
No Linters Available
Unit
No Unit Test Coverage
lbrabec retitled this revision from to new cli arg - variable value override.Aug 13 2014, 10:52 AM
lbrabec updated this object.
lbrabec edited the test plan for this revision. (Show Details)
lbrabec added a reviewer: jskladan.
kparal requested changes to this revision.Aug 13 2014, 11:35 AM
kparal added a reviewer: kparal.
kparal added a subscriber: kparal.

Thanks for the patch. Few adjustment proposals below. Also, could you please add unit tests?

libtaskotron/runner.py
47

It should be safer to evaluate into custom dictionaries instead of globals() and locals() (look at optional arguments for eval()).

48

Please add a debug output here - what gets overridden and to what value.

179

I'd rather keep -o reserved for future use. Overriding should not be that common, let's use just the long version.

Can you make more obvious in help that this is intended to override internal variables used for the runner and the task formula? It might not be clear here. Of course it doesn't make sense to override CLI variables, because that can be adjusted directly from the command line.

Also, it would help a lot to include an example of the option value. And note that it is actually evaluated by eval().

205–214

Let's say:

# process overrides

here to keep it consistent.

207

I might be wrong, but simple

if args['override'] is not None:

should be sufficient here. Or it will be an empty list if we change it to multi-value parameter.

208

I'd rather request -o to be specified multiple times in order to override multiple variables, than introduce a custom multi-value syntax - splitting by whitespace.

Splitting single values into var and value by = sign, that's fine. We can't probably make it more obvious here.

This revision now requires changes to proceed.Aug 13 2014, 11:35 AM
lbrabec updated this revision to Diff 586.Aug 13 2014, 2:39 PM
  • polishing
lbrabec updated this revision to Diff 587.Aug 13 2014, 3:41 PM
  • polishing 2
jskladan added a comment.Aug 14 2014, 7:07 AM

Is there any specific reason for using:

parser.add_argument("--override", action="append", default=[], ...)
...
if 'override' in args: # <----- this if clause is always True

Instead of

parser.add_argument("--override", action="append", ...)
...
if args['override'] is not None:

?

lbrabec updated this revision to Diff 588.Aug 14 2014, 9:41 AM
  • polishing 3
lbrabec updated this revision to Diff 589.Aug 14 2014, 9:54 AM
  • metavar additon
kparal accepted this revision.Aug 14 2014, 9:57 AM

Looks OK here.

This revision is now accepted and ready to land.Aug 14 2014, 9:57 AM
jskladan accepted this revision.Aug 14 2014, 10:12 AM
tflink accepted this revision.Aug 14 2014, 11:17 AM

LGTM

lbrabec added a comment.Aug 14 2014, 11:21 AM

Closed by commit 11434de (authored by @lbrabec).

lbrabec closed this revision.Aug 14 2014, 11:21 AM

Revision Contents

PathPackages
Mlibtaskotron/runner.py (21 lines)
Mtesting/test_runner.py (46 lines)

Diff 589

View Options

libtaskotron/runner.py

Show All 36 Lines35 def run(self):
37 37​
38 if not self.workdir: # create temporary workdir if needed 38​ if not self.workdir: # create temporary workdir if needed
39 self.workdir = tempfile.mkdtemp(prefix="task-", 39​ self.workdir = tempfile.mkdtemp(prefix="task-",
40 dir=config.get_config().tmpdir) 40​ dir=config.get_config().tmpdir)
41 log.debug("Current workdir: %s", self.workdir) 41​ log.debug("Current workdir: %s", self.workdir)
42 self.envdata['workdir'] = self.workdir 42​ self.envdata['workdir'] = self.workdir
43 self.envdata['checkname'] = self.taskdata['name'] 43​ self.envdata['checkname'] = self.taskdata['name']
44 44​
45​ #override variable values
46​ for var, val in self.envdata['override']:
47​ log.debug("Overriding variable %s, new value: %s", var, val)
kparalUnsubmitted
Not Done

It should be safer to evaluate into custom dictionaries instead of globals() and locals() (look at optional arguments for eval()).

48​ self.envdata[var] = eval(val, {}, {})
kparalUnsubmitted
Not Done

Please add a debug output here - what gets overridden and to what value.

49​
45 self.do_actions() 50​ self.do_actions()
46 51​
47 def _load_directive(self, directive_name, directive_dir=None): 52​ def _load_directive(self, directive_name, directive_dir=None):
48 # look in default path if nothing is specified 53​ # look in default path if nothing is specified
49 if not directive_dir: 54​ if not directive_dir:
50 directive_dir = os.path.join(os.path.dirname(__file__), 55​ directive_dir = os.path.join(os.path.dirname(__file__),
51 'directives') 56​ 'directives')
52 57​
▲ Show 20 Lines • Show All 113 Lines • ▼ Show 20 Line(s)162def get_argparser():
166 parser.add_argument("-t", "--type", 171​ parser.add_argument("-t", "--type",
167 choices=_ITEM_TYPES, 172​ choices=_ITEM_TYPES,
168 help="type of --item argument") 173​ help="type of --item argument")
169 parser.add_argument("-j", "--jobid", default="-1", 174​ parser.add_argument("-j", "--jobid", default="-1",
170 help="optional job identifier used to render log urls") 175​ help="optional job identifier used to render log urls")
171 parser.add_argument("-d", "--debug", action="store_true", 176​ parser.add_argument("-d", "--debug", action="store_true",
172 help="Enable debug output " 177​ help="Enable debug output "
173 "(set logging level to 'DEBUG')") 178​ "(set logging level to 'DEBUG')")
179​ parser.add_argument("--override", action="append", default=[],
kparalUnsubmitted
Not Done

I'd rather keep -o reserved for future use. Overriding should not be that common, let's use just the long version.

Can you make more obvious in help that this is intended to override internal variables used for the runner and the task formula? It might not be clear here. Of course it doesn't make sense to override CLI variables, because that can be adjusted directly from the command line.

Also, it would help a lot to include an example of the option value. And note that it is actually evaluated by eval().

180​ metavar='VAR=VALUE',
181​ help="override internal variable values used in runner "
182​ "and the task formula. Value itself is evaluated "
183​ "by eval(). This option can be used multiple times. "
184​ "Example: --override \"workdir='/some/dir/'\"")
185​
174 return parser 186​ return parser
175 187​
176 188​
177def process_args(args): 189​def process_args(args):
178 """ Processes raw input args and converts them into specific data types that 190​ """ Processes raw input args and converts them into specific data types that
179 can be used by tasks. This includes e.g. creating new args based on 191​ can be used by tasks. This includes e.g. creating new args based on
180 (item, item_type) pairs, or adjusting selected architecture. 192​ (item, item_type) pairs, or adjusting selected architecture.
181 193​
182 :param dict args: dictionary of raw arguments 194​ :param dict args: dictionary of raw arguments
183 :returns: dict of args with appended specific data 195​ :returns: dict of args with appended specific data
184 """ 196​ """
185 197​
186 # process item + type 198​ # process item + type
187 if args['type'] in _ITEM_TYPES: 199​ if args['type'] in _ITEM_TYPES:
188 args[args['type']] = args['item'] 200​ args[args['type']] = args['item']
189 201​
190 # process arch 202​ # process arch
191 if args['arch'] is None: 203​ if args['arch'] is None:
192 args['arch'] = ['noarch'] 204​ args['arch'] = ['noarch']
193 205​
206​ # using list of tuples instead of dictionary so the args['override'] is
207​ # still list at the end of process_args() - for better testability
kparalUnsubmitted
Not Done

I might be wrong, but simple

if args['override'] is not None:

should be sufficient here. Or it will be an empty list if we change it to multi-value parameter.

208​ override = []
kparalUnsubmitted
Not Done

I'd rather request -o to be specified multiple times in order to override multiple variables, than introduce a custom multi-value syntax - splitting by whitespace.

Splitting single values into var and value by = sign, that's fine. We can't probably make it more obvious here.

209​ for var in args['override']:
210​ name = var.split('=')[0]
211​ value = var.split('=')[1]
212​ override.append((name, value))
213​ args['override'] = override
214​
kparalUnsubmitted
Not Done

Let's say:

# process overrides

here to keep it consistent.

194 return args 215​ return args
195 216​
196 217​
197def main(): 218​def main():
198 # Preliminary initialization of logging, so all messages before regular 219​ # Preliminary initialization of logging, so all messages before regular
199 # initialization can be logged to stream. 220​ # initialization can be logged to stream.
200 # FIXME: Remove this once this ticket is resolved: 221​ # FIXME: Remove this once this ticket is resolved:
201 # https://phab.qadevel.cloud.fedoraproject.org/T273 222​ # https://phab.qadevel.cloud.fedoraproject.org/T273
Show All 27 Lines
View Options

testing/test_runner.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2014, Red Hat, Inc. 2​# Copyright 2009-2014, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6import pytest 6​import pytest
7import os 7​import os
8import sys 8​import sys
9import copy 9​import copy
10​from dingus import Dingus
10 11​
11from libtaskotron import runner 12​from libtaskotron import runner
12from libtaskotron import exceptions as exc 13​from libtaskotron import exceptions as exc
13 14​
14 15​
15class TestRunnerInputVerify(): 16​class TestRunnerInputVerify():
16 def test_yamlrunner_valid_input(self): 17​ def test_yamlrunner_valid_input(self):
17 ref_cmd = '-i foo-1.2-3.fc99 -t koji_build -a x86_64 footask.yml' 18​ ref_cmd = '-i foo-1.2-3.fc99 -t koji_build -a x86_64 footask.yml'
▲ Show 20 Lines • Show All 320 Lines • ▼ Show 20 Line(s)
338 339​
339 340​
340class TestRunnerProcessArgs(): 341​class TestRunnerProcessArgs():
341 342​
342 def test_process_args_koji_build(self): 343​ def test_process_args_koji_build(self):
343 ref_input = { 'arch': ['x86_64'], 344​ ref_input = { 'arch': ['x86_64'],
344 'item': 'foo-1.2-3.fc99', 345​ 'item': 'foo-1.2-3.fc99',
345 'type': 'koji_build', 346​ 'type': 'koji_build',
346 'task': 'sometask.yml'} 347​ 'task': 'sometask.yml',
348​ 'override': []}
347 349​
348 ref_args = copy.deepcopy(ref_input) 350​ ref_args = copy.deepcopy(ref_input)
349 ref_args['koji_build'] = 'foo-1.2-3.fc99' 351​ ref_args['koji_build'] = 'foo-1.2-3.fc99'
350 352​
351 test_args = runner.process_args(ref_input) 353​ test_args = runner.process_args(ref_input)
352 354​
353 assert test_args == ref_args 355​ assert test_args == ref_args
354 356​
355 def test_process_args_bodhi_id(self): 357​ def test_process_args_bodhi_id(self):
356 ref_input = { 'arch': ['x86_64'], 358​ ref_input = { 'arch': ['x86_64'],
357 'item': 'foo-1.2-3.fc99', 359​ 'item': 'foo-1.2-3.fc99',
358 'type': 'bodhi_id', 360​ 'type': 'bodhi_id',
359 'task': 'sometask.yml'} 361​ 'task': 'sometask.yml',
362​ 'override': []}
360 363​
361 ref_args = copy.deepcopy(ref_input) 364​ ref_args = copy.deepcopy(ref_input)
362 ref_args['bodhi_id'] = 'foo-1.2-3.fc99' 365​ ref_args['bodhi_id'] = 'foo-1.2-3.fc99'
363 366​
364 test_args = runner.process_args(ref_input) 367​ test_args = runner.process_args(ref_input)
365 368​
366 assert test_args == ref_args 369​ assert test_args == ref_args
367 370​
368 def test_process_args_koji_tag(self): 371​ def test_process_args_koji_tag(self):
369 ref_input = { 'arch': ['x86_64'], 372​ ref_input = { 'arch': ['x86_64'],
370 'item': 'dist-fc99-updates', 373​ 'item': 'dist-fc99-updates',
371 'type': 'koji_tag', 374​ 'type': 'koji_tag',
372 'task': 'sometask.yml'} 375​ 'task': 'sometask.yml',
376​ 'override': []}
373 377​
374 ref_args = copy.deepcopy(ref_input) 378​ ref_args = copy.deepcopy(ref_input)
375 ref_args['koji_tag'] = 'dist-fc99-updates' 379​ ref_args['koji_tag'] = 'dist-fc99-updates'
376 380​
377 test_args = runner.process_args(ref_input) 381​ test_args = runner.process_args(ref_input)
378 382​
379 assert test_args == ref_args 383​ assert test_args == ref_args
380 384​
381 def test_process_args_koji_tag_multiple_arches(self): 385​ def test_process_args_koji_tag_multiple_arches(self):
382 ref_input = { 'arch': ['x86_64', 'i386', 'noarch'], 386​ ref_input = { 'arch': ['x86_64', 'i386', 'noarch'],
383 'item': 'dist-fc99-updates', 387​ 'item': 'dist-fc99-updates',
384 'type': 'koji_tag', 388​ 'type': 'koji_tag',
385 'task': 'sometask.yml'} 389​ 'task': 'sometask.yml',
390​ 'override': []}
386 391​
387 ref_args = copy.deepcopy(ref_input) 392​ ref_args = copy.deepcopy(ref_input)
388 ref_args['koji_tag'] = 'dist-fc99-updates' 393​ ref_args['koji_tag'] = 'dist-fc99-updates'
389 394​
390 test_args = runner.process_args(ref_input) 395​ test_args = runner.process_args(ref_input)
391 396​
392 assert test_args == ref_args 397​ assert test_args == ref_args
393 398​
394 def test_process_args_no_arch(self): 399​ def test_process_args_no_arch(self):
395 ref_input = { 'type': 'invalid', 400​ ref_input = { 'type': 'invalid',
396 'arch': None} 401​ 'arch': None,
402​ 'override': []}
397 ref_args = copy.deepcopy(ref_input) 403​ ref_args = copy.deepcopy(ref_input)
398 ref_args['arch'] = ['noarch'] 404​ ref_args['arch'] = ['noarch']
399 405​
400 test_args = runner.process_args(ref_input) 406​ test_args = runner.process_args(ref_input)
401 assert test_args == ref_args 407​ assert test_args == ref_args
402 408​
409​
410​class TestRunnerCheckOverride():
411​
412​ def test_override_existing(self):
413​ ref_input = { 'arch': ['x86_64'],
414​ 'type': 'koji_build',
415​ 'item': 'foo-1.2-3.fc99',
416​ 'override': ["workdir='/some/dir/'"]}
417​
418​ ref_inputdata = runner.process_args(ref_input)
419​
420​ test_runner = runner.Runner(Dingus(), ref_inputdata)
421​ test_runner.do_actions = lambda: None
422​ test_runner.run()
423​
424​ assert test_runner.envdata['workdir'] == "/some/dir/"
425​
426​ def test_override_nonexisting(self):
427​ ref_input = { 'arch': ['x86_64'],
428​ 'type': 'koji_build',
429​ 'item': 'foo-1.2-3.fc99',
430​ 'override': ["friendship='is magic'"]}
431​
432​ ref_inputdata = runner.process_args(ref_input)
433​
434​ test_runner = runner.Runner(Dingus(), ref_inputdata)
435​ test_runner.do_actions = lambda: None
436​ test_runner.run()
437​
438​ assert test_runner.envdata['friendship'] == "is magic"