Update cloudbuild substitution rules

Doug Greiman · Doug Greiman · commit 4ed577487b34 · 2017-03-15T16:52:02.000-07:00
* Escaping $ is done with $$ not \$
* Undefined builtin substitutions are now an error, instead of
  being replaced by the empty string
diff --git a/scripts/local_cloudbuild.py b/scripts/local_cloudbuild.py
@@ -49,12 +49,13 @@
 # Container Builder substitutions
 # https://cloud.google.com/container-builder/docs/api/build-requests#substitutions
 SUBSTITUTION_REGEX = re.compile(r"""(?x)
-    (?<!\\)                # Don't match if backslash before dollar sign
-    \$                     # Dollar sign
+    [$]                    # Dollar sign
     (
         [A-Z_][A-Z0-9_]*   # Variable name, no curly brackets
         |
         {[A-Z_][A-Z0-9_]*} # Variable name, with curly brackets
+        |
+        [$]                # $$, translated to a single literal $
     )
 """)
 
@@ -128,16 +129,14 @@ def sub(match):
         if variable_name[0] == '{':
             # Strip curly brackets
             variable_name = variable_name[1:-1]
-        if variable_name not in substitutions:
-            if variable_name.startswith('_'):
-                # User variables must be set
-                raise ValueError(
-                    'Variable "{}" used without being defined.  Try adding '
-                    'it to the --substitutions flag'.format(
-                        variable_name))
-            else:
-                # Builtin variables are silently turned into empty strings
-                value = ''
+        if variable_name == '$':
+            value = '$'
+        elif variable_name not in substitutions:
+            # Variables must be set
+            raise ValueError(
+                'Variable "{}" used without being defined.  Try adding '
+                'it to the --substitutions flag'.format(
+                    variable_name))
         else:
             value = substitutions.get(variable_name)
         substitutions_used.add(variable_name)
diff --git a/scripts/local_cloudbuild_test.py b/scripts/local_cloudbuild_test.py
@@ -123,16 +123,12 @@ def test_sub_and_quote(self):
             ('', {}, "''", []),
             # No substitutions
             ('a', {}, 'a', []),
-            # Unused builtin substitutions are fine
+            # Unused substitition (ok here but error in generate_script)
             ('a', {'FOO':'foo'}, 'a', []),
-            # Unused user substitition (ok here but error in generate_script)
             ('a', {'_FOO':'_foo'}, 'a', []),
             # Defined builtin substitution
             ('a$FOOb', {'FOO':'foo'}, 'afoob', ['FOO']),
             ('a${FOO}b', {'FOO':'foo'}, 'afoob', ['FOO']),
-            # Undefined builtin substitution
-            ('a$FOOb', {}, 'ab', ['FOO']),
-            ('a${FOO}b', {}, 'ab', ['FOO']),
             # Defined user substitution
             ('a$_FOOb', {'_FOO':'_foo'}, 'a_foob', ['_FOO']),
             ('a${_FOO}b', {'_FOO':'_foo'}, 'a_foob', ['_FOO']),
@@ -153,6 +149,9 @@ def test_sub_and_quote(self):
                 self.assertEqual(used, set(expected_used))
 
         invalid_cases = (
+            # Undefined builtin substitution
+            ('a$FOOb', {}),
+            ('a${FOO}b', {}),
             # Undefined user substitution
             ('a$_FOOb', {}),
             ('a${_FOO}b', {}),
@@ -293,8 +292,8 @@ def test_generate_command(self):
         self.assertIn("'a builtin substitution'", command)
 
         step = base_step._replace(name='a $UNSET_BUILTIN substitution')
-        command = local_cloudbuild.generate_command(step, subs, set())
-        self.assertIn("'a  substitution'", command)
+        with self.assertRaises(ValueError):
+            command = local_cloudbuild.generate_command(step, subs, set())
 
         step = base_step._replace(name='a $_USER substitution')
         command = local_cloudbuild.generate_command(step, subs, set())
@@ -308,6 +307,10 @@ def test_generate_command(self):
         command = local_cloudbuild.generate_command(step, subs, set())
         self.assertIn("'a curly brace builtin substitution'", command)
 
+        step = base_step._replace(name='an escaped $$ or $$$$ or $$BUILTIN or $${BUILTIN} is unescaped')
+        command = local_cloudbuild.generate_command(step, subs, set())
+        self.assertIn("'an escaped $ or $$ or $BUILTIN or ${BUILTIN} is unescaped'", command)
+
     def test_generate_script_golden(self):
         config_name = 'cloudbuild_ok.yaml'
         config = os.path.join(self.testdata_dir, config_name)
