Skip EnsureRenderedStringPattern preprocessor if parameter has verbatim directives
#19
Conversation
hyungoos
commented
Jan 8, 2019
hyungoos
commented
- Log warning message instead
…s have verbatim directives
|
Patch for #4 |
There was a problem hiding this comment.
Hey @hyungoos thanks for contributing this fix! I had a minor request that I think will make this a little more generalizable to include some cases I had been considering for future work. If you wouldn't mind updating the PR, we can merge ASAP.
| @@ -110,6 +110,14 @@ def process_arg(self, arg, node, raw_args): | |||
| regex.pattern) | |||
| return arg | |||
|
|
|||
| VERBATIM_REGEX = '<<item_name>>|<<item.name>>|<<item.value>>' | |||
There was a problem hiding this comment.
Hey so one feature that I would like to support is the ability to actually require that the argument be a (or contain a) verbatim string... For example, there are some operators that take methods as arguments, and the only way we can support those right now is for the parameter passed to the operator to be a lambda (at the moment we do not enforce any restrictions on these, but we probably should).
So could we do the VERBATIM_REGEX check only if the regex.match() check fails? Maybe change the code to something like (starting at line 114 and replacing all of the rest of the method):
if regex.match(rendered_arg):
return arg
VERBATIM_REGEX = ...
if re.compile(VERBATIM_REGEX).search(rendered_arg):
logger.warning(...)
return arg
raise Exception('Invalid argument...')There was a problem hiding this comment.
Oh and also, I think we can loosen the definition of the VERBATIM_STRING regex to something like <<.+>>, because generally speaking it's possible for any verbatim string to be present, and we can't check any of them.
There was a problem hiding this comment.
Thanks for the comments. Done!
| assert renderer.process_arg( | ||
| "my-cluster-<<item.value>>", None, {}, | ||
| ) == "my-cluster-<<item.value>>" | ||
| warning_mock.assert_called_once() |
|
@hyungoos sadly Travis is failing with a minor linter error, but it is also taking many minutes to launch new jobs right now. I am going to merge with the broken test, push the fix myself, and then i will deploy the new version. |
