(Resending for dalecurtis; Patchwork didn't pick up the patch, and I suspect
it's because it doesn't recognize the file extension)
All,
The "host list", "host create", and "job create" command line options for
the atest CLI module do not support labels with commas. In our deployment we
have several labels which contain commas, leading to some bad parsing when
trying to use the CLI tools. I've added quoting support to the
topic_common.item_parse_info.get_values.__get_items(...) function to fix
this.
For example, previously 'atest host list --label label0,"comma,label"' would
not be parsed properly into two labels. Now the labels are parsed as
'label0' and 'comma,label'.
Any option using topic_common.item_parse_info gets this behavior for free.
Specifically for our needs, I've changed the "atest host list" and "atest
job create" methods from using <string>.split(',') to
topic_common.item_parse_info for labels.
Implementation wise, I modified the
topic_common.item_parse_info.get_values.__get_items(...) regex so that it
wouldn't split up items in quotes and changed the stripping so that leading
and trailing quotes were removed from the split items. The new regex is ([,
]|\\\".*?\\\".*?) if space splitting is enabled and ([,]|\\\".*?\\\".*?)
otherwise.
Testing was done through 18 new unit tests, running the gamut of good quotes
and mismatched quotes at the parser level as well as individual unit tests
for the "host list", "host create", and "job create" methods.
All unit tests pass, but if there are any deployments out there that were
using the CLI tools and have quotes in their labels (or any option relying
on topic_common.item_parse_info), they will no longer parse properly.
Another more minor quibble is consistency from a user point of view. I've
only added support for quotes to the topic_common.item_parse_info class, but
there are a couple other areas which are still using <string>.split(',') to
parse their options. Specifically status names, host names, test names, and
dependencies in the host.py and job.py files. I've left these alone for now,
but if anyone objects I can roll them in.
I've attached the patch file for the changes, but if you prefer a more GUI
driven experience, you can see the change list here:
http://codereview.chromium.org/2740001/show
Risk: Low-Medium (CLI infrastructure and tests modified)
Visibility: CLI users
Regards,
Dale Curtis
Software Engineer in Test @ Google
Signed-off-by: Dale Curtis <[email protected]>
Index: cli/host.py
diff --git a/cli/host.py b/cli/host.py
index e5bfd10810602ecf6daf76ff09a7629689841665..46013717cfd79173296278e61faaf80694c13540 100644
--- a/cli/host.py
+++ b/cli/host.py
@@ -128,8 +128,11 @@ class host_list(action_common.atest_list, host):
def parse(self):
"""Consume the specific options"""
- (options, leftover) = super(host_list, self).parse()
- self.labels = options.label
+ label_info = topic_common.item_parse_info(attribute_name='labels',
+ inline_option='label')
+
+ (options, leftover) = super(host_list, self).parse([label_info])
+
self.status = options.status
self.acl = options.acl
self.user = options.user
@@ -151,15 +154,12 @@ class host_list(action_common.atest_list, host):
check_results['hostname__in'] = 'hostname'
if self.labels:
- labels = self.labels.split(',')
- labels = [label.strip() for label in labels if label.strip()]
-
- if len(labels) == 1:
+ if len(self.labels) == 1:
# This is needed for labels with wildcards (x86*)
- filters['labels__name__in'] = labels
+ filters['labels__name__in'] = self.labels
check_results['labels__name__in'] = None
else:
- filters['multiple_labels'] = labels
+ filters['multiple_labels'] = self.labels
check_results['multiple_labels'] = None
if self.status:
Index: cli/host_unittest.py
diff --git a/cli/host_unittest.py b/cli/host_unittest.py
index 197e9f3cee426f6fd80575d8c8ad04c1d5642e77..bd68c040e7fd878f4cf8bcf00c5ab1e1dff94745 100755
--- a/cli/host_unittest.py
+++ b/cli/host_unittest.py
@@ -70,7 +70,7 @@ class host_list_unittest(cli_mock.cli_unittest):
hl = host.host_list()
sys.argv = ['atest', '--label', 'label0']
(options, leftover) = hl.parse()
- self.assertEqual('label0', hl.labels)
+ self.assertEqual(['label0'], hl.labels)
self.assertEqual(leftover, [])
@@ -78,7 +78,39 @@ class host_list_unittest(cli_mock.cli_unittest):
hl = host.host_list()
sys.argv = ['atest', '--label', 'label0,label2']
(options, leftover) = hl.parse()
- self.assertEqual('label0,label2', hl.labels)
+ self.assertEqualNoOrder(['label0', 'label2'], hl.labels)
+ self.assertEqual(leftover, [])
+
+
+ def test_parse_with_quoted_label(self):
+ hl = host.host_list()
+ sys.argv = ['atest', '--label', '"label 0"']
+ (options, leftover) = hl.parse()
+ self.assertEqual(['label 0'], hl.labels)
+ self.assertEqual(leftover, [])
+
+
+ def test_parse_with_quoted_multi_labels(self):
+ hl = host.host_list()
+ sys.argv = ['atest', '--label', '"label 0","label 2"']
+ (options, leftover) = hl.parse()
+ self.assertEqualNoOrder(['label 0', 'label 2'], hl.labels)
+ self.assertEqual(leftover, [])
+
+
+ def test_parse_with_quoted_multi_labels_and_commas(self):
+ hl = host.host_list()
+ sys.argv = ['atest', '--label', '"label,0","label, 2"']
+ (options, leftover) = hl.parse()
+ self.assertEqualNoOrder(['label,0', 'label, 2'], hl.labels)
+ self.assertEqual(leftover, [])
+
+
+ def test_parse_with_bad_quote_multi_labels_and_commas(self):
+ hl = host.host_list()
+ sys.argv = ['atest', '--label', '"label,0","label, 2, label3']
+ (options, leftover) = hl.parse()
+ self.assertEqualNoOrder(['label,0', 'label', '2', 'label3'], hl.labels)
self.assertEqual(leftover, [])
@@ -90,7 +122,7 @@ class host_list_unittest(cli_mock.cli_unittest):
(options, leftover) = hl.parse()
self.assertEqualNoOrder(['host0', 'host1','host3', 'host4'],
hl.hosts)
- self.assertEqual('label0', hl.labels)
+ self.assertEqual(['label0'], hl.labels)
self.assertEqual(leftover, [])
mfile.clean()
@@ -277,8 +309,8 @@ class host_list_unittest(cli_mock.cli_unittest):
def test_execute_list_filter_multi_labels(self):
self.run_cmd(argv=['atest', 'host', 'list',
'-b', 'label3,label2', '--ignore_site_file'],
- rpcs=[('get_hosts', {'multiple_labels': ['label3',
- 'label2']},
+ rpcs=[('get_hosts', {'multiple_labels': ['label2',
+ 'label3']},
True,
[{u'status': u'Ready',
u'hostname': u'host1',
@@ -309,8 +341,8 @@ class host_list_unittest(cli_mock.cli_unittest):
self.run_cmd(argv=['atest', 'host', 'list',
'-b', 'label3,label2, label4',
'--ignore_site_file'],
- rpcs=[('get_hosts', {'multiple_labels': ['label3',
- 'label2',
+ rpcs=[('get_hosts', {'multiple_labels': ['label2',
+ 'label3',
'label4']},
True,
[{u'status': u'Ready',
@@ -355,8 +387,8 @@ class host_list_unittest(cli_mock.cli_unittest):
def test_execute_list_filter_multi_labels_no_results(self):
self.run_cmd(argv=['atest', 'host', 'list',
'-b', 'label3,label2, ', '--ignore_site_file'],
- rpcs=[('get_hosts', {'multiple_labels': ['label3',
- 'label2']},
+ rpcs=[('get_hosts', {'multiple_labels': ['label2',
+ 'label3']},
True,
[])],
out_words_ok=[],
@@ -1339,5 +1371,61 @@ class host_create_unittest(cli_mock.cli_unittest):
out_words_ok=['host0', 'host1'])
+ def test_execute_create_muliple_hosts_label_quotes(self):
+ self.run_cmd(argv=['atest', 'host', 'create',
+ '-b', 'label0,"label,1","label 2"',
+ '--acls', 'acl0', 'host0', 'host1',
+ '--ignore_site_file'],
+ rpcs=[('get_labels', {'name': 'label0'},
+ True,
+ [{u'id': 4,
+ u'platform': 0,
+ u'name': u'label0',
+ u'invalid': False,
+ u'kernel_config': u''}]),
+ ('get_labels', {'name': 'label,1'},
+ True,
+ [{u'id': 4,
+ u'platform': 0,
+ u'name': u'label,1',
+ u'invalid': False,
+ u'kernel_config': u''}]),
+ ('get_labels', {'name': 'label 2'},
+ True,
+ [{u'id': 4,
+ u'platform': 0,
+ u'name': u'label 2',
+ u'invalid': False,
+ u'kernel_config': u''}]),
+ ('get_acl_groups', {'name': 'acl0'},
+ True, []),
+ ('add_acl_group', {'name': 'acl0'},
+ True, 5),
+ ('add_host', {'hostname': 'host1',
+ 'status': 'Ready',
+ 'locked': True},
+ True, 42),
+ ('host_add_labels', {'id': 'host1',
+ 'labels': ['label0', 'label,1',
+ 'label 2']},
+ True, None),
+ ('add_host', {'hostname': 'host0',
+ 'status': 'Ready',
+ 'locked': True},
+ True, 42),
+ ('host_add_labels', {'id': 'host0',
+ 'labels': ['label0', 'label,1',
+ 'label 2']},
+ True, None),
+ ('acl_group_add_hosts',
+ {'id': 'acl0', 'hosts': ['host1', 'host0']},
+ True, None),
+ ('modify_host', {'id': 'host1', 'locked': False},
+ True, None),
+ ('modify_host', {'id': 'host0', 'locked': False},
+ True, None)],
+ out_words_ok=['host0', 'host1'])
+
+
if __name__ == '__main__':
unittest.main()
Index: cli/job.py
diff --git a/cli/job.py b/cli/job.py
index 276f9b106b3cd15c180c33bdfa722731694c6305..a5122be947932334f5b7baf4aea8f2f7d7a55df2 100644
--- a/cli/job.py
+++ b/cli/job.py
@@ -307,10 +307,12 @@ class job_create_or_clone(action_common.atest_create, job):
use_leftover=True)
oth_info = topic_common.item_parse_info(attribute_name='one_time_hosts',
inline_option='one_time_hosts')
+ label_info = topic_common.item_parse_info(attribute_name='labels',
+ inline_option='labels')
options, leftover = super(job_create_or_clone,
- self).parse([host_info, job_info, oth_info],
- req_items='jobname')
+ self).parse([host_info, job_info, oth_info,
+ label_info], req_items='jobname')
self.data = {}
if len(self.jobname) > 1:
self.invalid_syntax('Too many arguments specified, only expected '
@@ -323,11 +325,9 @@ class job_create_or_clone(action_common.atest_create, job):
if self.one_time_hosts:
self.data['one_time_hosts'] = self.one_time_hosts
- if options.labels:
- labels = options.labels.split(',')
- labels = [label.strip() for label in labels if label.strip()]
+ if self.labels:
label_hosts = self.execute_rpc(op='get_hosts',
- multiple_labels=labels)
+ multiple_labels=self.labels)
for host in label_hosts:
self.hosts.append(host['hostname'])
Index: cli/job_unittest.py
diff --git a/cli/job_unittest.py b/cli/job_unittest.py
index 15b820e6e34a54c3a48614b599a2d0a6c6f9c1cf..6f1e8c3010581d9a0c29d29e1455e8f94ed459b3 100755
--- a/cli/job_unittest.py
+++ b/cli/job_unittest.py
@@ -1163,6 +1163,50 @@ class job_create_unittest(cli_mock.cli_unittest):
file_temp.clean()
+ def test_execute_create_job_with_label_quotes_and_duplicate_hosts(self):
+ data = self.data.copy()
+ data['hosts'] = ['host1', 'host0']
+ file_temp = cli_mock.create_file(self.ctrl_file)
+ self.run_cmd(argv=['atest', 'job', 'create', '-f', file_temp.name,
+ 'test_job0', '-m', 'host0,host1', '-b',
+ 'label1,"label,2"', '--ignore_site_file'],
+ rpcs=[('get_hosts', {'multiple_labels': ['label1',
+ 'label,2']}, True,
+ [{u'status': u'Running', u'lock_time': None,
+ u'hostname': u'host1', u'locked': False,
+ u'locked_by': None, u'invalid': False, u'id': 42,
+ u'labels': [u'label1', u'label,2'], u'platform':
+ u'Warp18_Diskfull', u'protection':
+ u'Repair software only', u'dirty':
+ True, u'synch_id': None}]),
+ ('create_job', data, True, 42)],
+ out_words_ok=['test_job0', 'Created'],
+ out_words_no=['Uploading', 'Done'])
+ file_temp.clean()
+
+
+ def test_execute_create_job_with_label_bad_quotes_and_duplicate_hosts(self):
+ data = self.data.copy()
+ data['hosts'] = ['host1', 'host0']
+ file_temp = cli_mock.create_file(self.ctrl_file)
+ self.run_cmd(argv=['atest', 'job', 'create', '-f', file_temp.name,
+ 'test_job0', '-m', 'host0,host1', '-b',
+ 'label1,"label,2,label3', '--ignore_site_file'],
+ rpcs=[('get_hosts', {'multiple_labels': ['label1',
+ 'label3', '2', 'label']}, True,
+ [{u'status': u'Running', u'lock_time': None,
+ u'hostname': u'host1', u'locked': False,
+ u'locked_by': None, u'invalid': False, u'id': 42,
+ u'labels': [u'label1', u'label', u'2', u'label3'],
+ u'platform': u'Warp18_Diskfull', u'protection':
+ u'Repair software only', u'dirty':
+ True, u'synch_id': None}]),
+ ('create_job', data, True, 42)],
+ out_words_ok=['test_job0', 'Created'],
+ out_words_no=['Uploading', 'Done'])
+ file_temp.clean()
+
+
def _test_parse_hosts(self, args, exp_hosts=[], exp_meta_hosts=[]):
testjob = job.job_create_or_clone()
(hosts, meta_hosts) = testjob._parse_hosts(args)
Index: cli/topic_common.py
diff --git a/cli/topic_common.py b/cli/topic_common.py
index 958a0b9ffb0ca218c84a5363d3f493c9c547f160..12fad43f5b2ea9fd2c1c96ad5295b8f22e8fa919 100644
--- a/cli/topic_common.py
+++ b/cli/topic_common.py
@@ -56,7 +56,7 @@ High Level Algorithm:
"""
import getpass, optparse, os, pwd, re, socket, sys, textwrap, traceback
-import socket, urllib2
+import socket, string, urllib2
from autotest_lib.cli import rpc
from autotest_lib.frontend.afe.json_rpc import proxy
from autotest_lib.client.common_lib.test_utils import mock
@@ -176,9 +176,24 @@ class item_parse_info(object):
"""Returns the value for that attribute by accumualting all
the values found through the inline option, the parsing of the
file and the leftover"""
- def __get_items(string, split_re='[\s,]\s*'):
- return (item.strip() for item in re.split(split_re, string)
- if item)
+
+ def __get_items(input, split_spaces=True):
+ """Splits a string of comma separated items. By default quoted items
+ will not be split. I.e. Splitting 'a, "b,c", d' will yield
+ ['a', 'b,c', 'd']. If split_spaces is set to False spaces will not
+ be split. I.e. Splitting 'a b, "c,d", e' will yield
+ ['a b', 'c,d', 'e']"""
+
+ if (not split_spaces):
+ split = re.split(r'([,]|".*?".*?)', input)
+ else:
+ split = re.split(r'([, ]|".*?".*?)', input)
+
+ # Characters to strip on parse
+ strip_chars = string.whitespace + '",'
+
+ return (item.strip(strip_chars) for item in split if
+ item.strip(strip_chars))
if self.use_leftover:
add_on = leftover
@@ -191,7 +206,7 @@ class item_parse_info(object):
for items in add_on:
# Don't split on space here because the add-on
# may have some spaces (like the job name)
- result.update(__get_items(items, split_re='[,]'))
+ result.update(__get_items(items, split_spaces=False))
# Process the inline_option, if any
try:
Index: cli/topic_common_unittest.py
diff --git a/cli/topic_common_unittest.py b/cli/topic_common_unittest.py
index 5f4284e46c29453fa3cd815ebbfd3eaf4031c7b4..e9858bf6da1d6648a872e4b50756d2479845be6b 100755
--- a/cli/topic_common_unittest.py
+++ b/cli/topic_common_unittest.py
@@ -161,6 +161,27 @@ class item_parse_info_unittest(cli_mock.cli_unittest):
self.__test_parsing_flist_good(opt(), ['a', 'b', 'c'])
+ def test_file_list_quotes(self):
+ class opt(object):
+ flist_obj = cli_mock.create_file('a\n"b c d"\n"e f g"')
+ flist = flist_obj.name
+ self.__test_parsing_flist_good(opt(), ['a', 'b c d', 'e f g'])
+
+
+ def test_file_list_quotes_commas(self):
+ class opt(object):
+ flist_obj = cli_mock.create_file('a\n"b,c,d"\n"e f, g"')
+ flist = flist_obj.name
+ self.__test_parsing_flist_good(opt(), ['a', 'b,c,d', 'e f, g'])
+
+
+ def test_file_list_bad_quotes_commas(self):
+ class opt(object):
+ flist_obj = cli_mock.create_file('a\n"b,c,"""d\n"e f, g"')
+ flist = flist_obj.name
+ self.__test_parsing_flist_good(opt(), ['a', 'b,c', 'd', 'e f, g'])
+
+
def test_file_list_opt_list_one(self):
class opt(object):
inline = 'a'
@@ -191,6 +212,24 @@ class item_parse_info_unittest(cli_mock.cli_unittest):
self.__test_parsing_inline_good(opt(), ['a', 'b', 'c', 'd', 'e'])
+ def test_file_list_opt_list_quotes(self):
+ class opt(object):
+ inline = '"a b", c, d'
+ self.__test_parsing_inline_good(opt(), ['a b', 'c', 'd'])
+
+
+ def test_file_list_opt_list_quotes_commas(self):
+ class opt(object):
+ inline = '"a,b",c, d'
+ self.__test_parsing_inline_good(opt(), ['a,b', 'c', 'd'])
+
+
+ def test_file_list_opt_list_bad_quotes_commas(self):
+ class opt(object):
+ inline = '"a,b, c """ ,d, e"'
+ self.__test_parsing_inline_good(opt(), ['a,b, c', 'd', 'e'])
+
+
def test_file_list_add_on_space(self):
self.__test_parsing_leftover_good(['a','c','b'],
['a', 'b', 'c'])
@@ -211,6 +250,21 @@ class item_parse_info_unittest(cli_mock.cli_unittest):
['a', 'b', 'c', 'd'])
+ def test_file_list_add_on_quotes(self):
+ self.__test_parsing_leftover_good(['a', 'c', 'b,', '"d e f"'],
+ ['a', 'b', 'c', 'd e f'])
+
+
+ def test_file_list_add_on_quotes_commas(self):
+ self.__test_parsing_leftover_good(['a', 'c', 'b,', '"d, e, f"'],
+ ['a', 'b', 'c', 'd, e, f'])
+
+
+ def test_file_list_add_on_bad_quotes_commas(self):
+ self.__test_parsing_leftover_good(['a', 'c', 'b,', '"d, e, f','"""'],
+ ['a', 'b', 'c', 'd', 'e', 'f'])
+
+
def test_file_list_all_opt(self):
class opt(object):
flist_obj = cli_mock.create_file('f\ng\nh\n')
@@ -258,6 +312,26 @@ class item_parse_info_unittest(cli_mock.cli_unittest):
'f', 'g', 'h', 'i', 'j'])
+ def test_file_list_all_opt_in_common_quotes(self):
+ class opt(object):
+ flist_obj = cli_mock.create_file('"a b c"\nd,e\nf\ng')
+ flist = flist_obj.name
+ inline = '"a b c",d h'
+ self.__test_parsing_all_good(opt(), ['i','j,d'],
+ ['a b c', 'd', 'e', 'f', 'g', 'h',
+ 'i', 'j'])
+
+
+ def test_file_list_all_opt_in_common_weird_bad_quotes(self):
+ class opt(object):
+ flist_obj = cli_mock.create_file('"a b c"\nd,e\nf\n"g,hij", ,, k,')
+ flist = flist_obj.name
+ inline = '"a b c",d h,"""ijk"'
+ self.__test_parsing_all_good(opt(), ['i','j,d'],
+ ['a b c', 'd', 'e', 'f', 'g,hij', 'h',
+ 'i', 'j', 'k', 'ijk'])
+
+
class atest_unittest(cli_mock.cli_unittest):
def setUp(self):
super(atest_unittest, self).setUp()
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
