Skip to content

Commit 89773e9

Browse files
author
Doug Greiman
committed
Validate that all user substitutions are used at least once
1 parent e6dfe0c commit 89773e9

2 files changed

Lines changed: 86 additions & 46 deletions

File tree

‎scripts/local_cloudbuild.py‎

Lines changed: 41 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,15 @@
112112
Step = collections.namedtuple('Step', 'args dir_ env name')
113113

114114

115-
def sub_and_quote(s, substitutions):
116-
"""Return a shell-escaped, variable substituted, version of the string s."""
115+
def sub_and_quote(s, substitutions, substitutions_used):
116+
"""Return a shell-escaped, variable substituted, version of the string s.
117+
118+
Args:
119+
s (str): Any string
120+
subs (dict): Substitution map to apply
121+
subs_used (set): Updated with names from `subs.keys()` when those
122+
substitutions are encountered in `s`
123+
"""
117124

118125
def sub(match):
119126
"""Perform a single substitution."""
@@ -133,6 +140,7 @@ def sub(match):
133140
value = ''
134141
else:
135142
value = substitutions.get(variable_name)
143+
substitutions_used.add(variable_name)
136144
return value
137145

138146
substituted_s = re.sub(SUBSTITUTION_REGEX, sub, s)
@@ -242,24 +250,29 @@ def get_step(raw_step):
242250
)
243251

244252

245-
def generate_command(step, subs):
253+
def generate_command(step, substitutions, substitutions_used):
246254
"""Generate a single shell command to run for a single cloudbuild step
247255
248256
Args:
249257
step (Step): Valid build step
250258
subs (dict): Substitution map to apply
259+
subs_used (set): Updated with names from `subs.keys()` when those
260+
substitutions are encountered in an element of `step`
251261
252262
Returns:
253263
[str]: A single shell command, expressed as a list of quoted tokens.
254264
"""
255-
quoted_args = [sub_and_quote(arg, subs) for arg in step.args]
265+
quoted_args = [sub_and_quote(arg, substitutions, substitutions_used)
266+
for arg in step.args]
256267
quoted_env = []
257268
for env in step.env:
258-
quoted_env.extend(['--env', sub_and_quote(env, subs)])
259-
quoted_name = sub_and_quote(step.name, subs)
269+
quoted_env.extend(['--env', sub_and_quote(env, substitutions,
270+
substitutions_used)])
271+
quoted_name = sub_and_quote(step.name, substitutions, substitutions_used)
260272
workdir = '/workspace'
261273
if step.dir_:
262-
workdir = os.path.join(workdir, sub_and_quote(step.dir_, subs))
274+
workdir = os.path.join(workdir, sub_and_quote(step.dir_, substitutions,
275+
substitutions_used))
263276
process_args = [
264277
'docker',
265278
'run',
@@ -284,16 +297,27 @@ def generate_script(cloudbuild):
284297
Returns:
285298
(str): Contents of shell script
286299
"""
287-
outfile = io.StringIO()
288-
outfile.write(BUILD_SCRIPT_HEADER)
289-
docker_commands = [generate_command(step, cloudbuild.substitutions)
290-
for step in cloudbuild.steps]
291-
for docker_command in docker_commands:
292-
line = ' '.join(docker_command) + '\n\n'
293-
outfile.write(line)
294-
outfile.write(BUILD_SCRIPT_FOOTER)
295-
s = outfile.getvalue()
296-
outfile.close()
300+
with io.StringIO() as outfile:
301+
outfile.write(BUILD_SCRIPT_HEADER)
302+
subs_used = set()
303+
docker_commands = [
304+
generate_command(step, cloudbuild.substitutions, subs_used)
305+
for step in cloudbuild.steps]
306+
for docker_command in docker_commands:
307+
line = ' '.join(docker_command) + '\n\n'
308+
outfile.write(line)
309+
outfile.write(BUILD_SCRIPT_FOOTER)
310+
s = outfile.getvalue()
311+
312+
# Check that all user variables were referenced at least once
313+
user_subs_unused = [name for name in cloudbuild.substitutions.keys()
314+
if name not in subs_used and name[0] == '_']
315+
if user_subs_unused:
316+
nice_list = '"' + '", "'.join(sorted(user_subs_unused)) + '"'
317+
raise ValueError(
318+
'User substitution variables {} were defined in the --substitution '
319+
'flag but never used in the cloudbuild file.'.format(nice_list))
320+
297321
return s
298322

299323

‎scripts/local_cloudbuild_test.py‎

Lines changed: 45 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -120,32 +120,37 @@ def setUp(self):
120120
def test_sub_and_quote(self):
121121
valid_cases = (
122122
# Empty string
123-
('', {}, "''"),
123+
('', {}, "''", []),
124124
# No substitutions
125-
('a', {}, 'a'),
126-
# Unused substitutions
127-
('a', {'FOO':'foo'}, 'a'),
125+
('a', {}, 'a', []),
126+
# Unused builtin substitutions are fine
127+
('a', {'FOO':'foo'}, 'a', []),
128+
# Unused user substitition (ok here but error in generate_script)
129+
('a', {'_FOO':'_foo'}, 'a', []),
128130
# Defined builtin substitution
129-
('a$FOOb', {'FOO':'foo'}, 'afoob'),
130-
('a${FOO}b', {'FOO':'foo'}, 'afoob'),
131+
('a$FOOb', {'FOO':'foo'}, 'afoob', ['FOO']),
132+
('a${FOO}b', {'FOO':'foo'}, 'afoob', ['FOO']),
131133
# Undefined builtin substitution
132-
('a$FOOb', {}, 'ab'),
133-
('a${FOO}b', {}, 'ab'),
134+
('a$FOOb', {}, 'ab', ['FOO']),
135+
('a${FOO}b', {}, 'ab', ['FOO']),
134136
# Defined user substitution
135-
('a$_FOOb', {'_FOO':'_foo'}, 'a_foob'),
136-
('a${_FOO}b', {'_FOO':'_foo'}, 'a_foob'),
137+
('a$_FOOb', {'_FOO':'_foo'}, 'a_foob', ['_FOO']),
138+
('a${_FOO}b', {'_FOO':'_foo'}, 'a_foob', ['_FOO']),
137139
# Multiple substitutions
138-
('$FOO${FOO}${BAR}$FOO', {'FOO':'foo', 'BAR':'bar'}, 'foofoobarfoo'),
140+
('$FOO${FOO}${BAR}$FOO', {'FOO':'foo', 'BAR':'bar'},
141+
'foofoobarfoo', ['FOO', 'BAR']),
139142
# Invalid names
140-
('a $ b', {}, "'a $ b'"),
141-
('a$foo b', {}, "'a$foo b'"),
142-
('a$0FOO b', {}, "'a$0FOO b'"),
143+
('a $ b', {}, "'a $ b'", []),
144+
('a$foo b', {}, "'a$foo b'", []),
145+
('a$0FOO b', {}, "'a$0FOO b'", []),
143146
)
144147
for valid_case in valid_cases:
145148
with self.subTest(valid_case=valid_case):
146-
s, subs, expected = valid_case
147-
actual = local_cloudbuild.sub_and_quote(s, subs)
149+
s, subs, expected, expected_used = valid_case
150+
used = set()
151+
actual = local_cloudbuild.sub_and_quote(s, subs, used)
148152
self.assertEqual(actual, expected)
153+
self.assertEqual(used, set(expected_used))
149154

150155
invalid_cases = (
151156
# Undefined user substitution
@@ -156,7 +161,8 @@ def test_sub_and_quote(self):
156161
with self.subTest(invalid_case=invalid_case):
157162
s, subs = invalid_case
158163
with self.assertRaises(ValueError):
159-
local_cloudbuild.sub_and_quote(s, subs)
164+
used = set()
165+
local_cloudbuild.sub_and_quote(s, subs, used)
160166

161167
def test_get_cloudbuild(self):
162168
args = argparse.Namespace(
@@ -237,7 +243,7 @@ def test_generate_command(self):
237243
name = 'aname',
238244
)
239245
subs = {'BUILTIN':'builtin', '_USER':'_user'}
240-
command = local_cloudbuild.generate_command(base_step, subs)
246+
command = local_cloudbuild.generate_command(base_step, subs, set())
241247
self.assertEqual(command, [
242248
'docker',
243249
'run',
@@ -260,49 +266,49 @@ def test_generate_command(self):
260266

261267
# dir specified
262268
step = base_step._replace(dir_='adir')
263-
command = local_cloudbuild.generate_command(step, subs)
269+
command = local_cloudbuild.generate_command(step, subs, set())
264270
self.assertIn('--workdir', command)
265271
self.assertIn('/workspace/adir', command)
266272

267273
# Shell quoting
268274
step = base_step._replace(args=['arg with \n newline'])
269-
command = local_cloudbuild.generate_command(step, subs)
275+
command = local_cloudbuild.generate_command(step, subs, set())
270276
self.assertIn("'arg with \n newline'", command)
271277

272278
step = base_step._replace(dir_='dir/ with space/')
273-
command = local_cloudbuild.generate_command(step, subs)
279+
command = local_cloudbuild.generate_command(step, subs, set())
274280
self.assertIn("/workspace/'dir/ with space/'", command)
275281

276282
step = base_step._replace(env=['env with space'])
277-
command = local_cloudbuild.generate_command(step, subs)
283+
command = local_cloudbuild.generate_command(step, subs, set())
278284
self.assertIn("'env with space'", command)
279285

280286
step = base_step._replace(name='a name')
281-
command = local_cloudbuild.generate_command(step, subs)
287+
command = local_cloudbuild.generate_command(step, subs, set())
282288
self.assertIn("'a name'", command)
283289

284290
# Variable substitution
285291
step = base_step._replace(name='a $BUILTIN substitution')
286-
command = local_cloudbuild.generate_command(step, subs)
292+
command = local_cloudbuild.generate_command(step, subs, set())
287293
self.assertIn("'a builtin substitution'", command)
288294

289295
step = base_step._replace(name='a $UNSET_BUILTIN substitution')
290-
command = local_cloudbuild.generate_command(step, subs)
296+
command = local_cloudbuild.generate_command(step, subs, set())
291297
self.assertIn("'a substitution'", command)
292298

293299
step = base_step._replace(name='a $_USER substitution')
294-
command = local_cloudbuild.generate_command(step, subs)
300+
command = local_cloudbuild.generate_command(step, subs, set())
295301
self.assertIn("'a _user substitution'", command)
296302

297303
step = base_step._replace(name='a $_UNSET_USER substitution')
298304
with self.assertRaises(ValueError):
299-
local_cloudbuild.generate_command(step, subs)
305+
local_cloudbuild.generate_command(step, subs, set())
300306

301307
step = base_step._replace(name='a curly brace ${BUILTIN} substitution')
302-
command = local_cloudbuild.generate_command(step, subs)
308+
command = local_cloudbuild.generate_command(step, subs, set())
303309
self.assertIn("'a curly brace builtin substitution'", command)
304310

305-
def test_generate_script(self):
311+
def test_generate_script_golden(self):
306312
config_name = 'cloudbuild_ok.yaml'
307313
config = os.path.join(self.testdata_dir, config_name)
308314
expected_output_script = os.path.join(self.testdata_dir, config_name + '_golden.sh')
@@ -331,6 +337,16 @@ def test_generate_script(self):
331337
with open(expected_output_script, 'r', encoding='utf8') as expected:
332338
self.assertEqual(actual, expected.read())
333339

340+
def test_generate_script_unused_user_substitution(self):
341+
cloudbuild = local_cloudbuild.CloudBuild(
342+
output_script='',
343+
run=False,
344+
steps=[],
345+
substitutions={'_FOO':'_foo'},
346+
)
347+
with self.assertRaisesRegexp(ValueError, 'User substitution variables'):
348+
actual = local_cloudbuild.generate_script(cloudbuild)
349+
334350
def test_make_executable(self):
335351
with tempfile.TemporaryDirectory(
336352
prefix='local_cloudbuild_test_') as tempdir:

0 commit comments

Comments
 (0)