Found another cornercase #224
Conversation
…onInString. The regex can consider multiple interpolation as only one.
There was a problem hiding this comment.
Ah, good find and test case.
Rather than adding that special-case check, I wonder if the right fix isn't to tweak INTERPOLATION_SYNTAX_REGEX. Instead of:
var INTERPOLATION_SYNTAX_REGEX = regexp.MustCompile(`\$\{.*?\}`)Try:
var INTERPOLATION_SYNTAX_REGEX = regexp.MustCompile(`\$\{[^\}]*?\}`)The change is that within the body of the ${...}, we capture everything other than a closing curly brace (}). I'd be curious of that passes all the tests without the extra FindAllString check.
|
It could work. I thought about that, but the problem with that kind of exclusion in regex is that it may have some undesired effect. Like that: Which is doubtful, but perfectly valid. |
|
Ah, good point. Do we have a test case for that? |
|
Not yet. I am trying something on regex101 to address this point https://regex101.com/r/MVuA1t/1 I must say that in our forked terragrunt version, we made several «enhancements» that are not ready to be submitted as PR. One of them is direct support for ${var.whatever} in terragrunt config. It would be nice if we can have a talk regarding those. |
Heh, nice. That said, there are diminishing returns here. A small tweak to the regex to avoid extra if-statements is good, but if the fix requires a huge, hard-to-understand regex, then an if-statement is probably easier to maintain.
Nice. We have a couple relevant discussions going on around this topic: |
|
@jocgir Let me know if you found a reasonable regex to fix this. If not, happy to merge the if-statement once we add the test case mentioned above. |
|
Yes, I think it would be preferabable to improve the regex (bug segmenting it in smaller, more readable parts). |
…e but error prone) Add a general error condition to handle non compliant interpolation syntax Removed the if statement in processSingleInterpolationInString which become useless Adapted the test functions Added a function to list terraform command that accept -input as a parameter. Add the documentation relative to get_terraform_commands_that_need_input
|
New regex looks promising! That said, it seems to still do a greedy capture, so is the reason that it works because you are now matching on word characters ( |
Had to fix HELPER_FUNCTION_SYNTAX_REGEX to be as tolerant as the generic INTERPOLATION_SYNTAX_REGEX regarding space Removed useless parenthesis in INTERPOLATION_SYNTAX_REGEX
|
Hi, I just add a test to ensure it. The \w+ just ensure that we got The tricky part is more in the parameters. There are still possible corner cases. Regex will never replace a real lexer/parser. |
|
Great, thanks! Merging now. |
There was a bug in certain circumstance whit processSingleInterpolationInString.
The regex can consider multiple interpolations as only one. The main problem is that INTERPOLATION_SYNTAX_REGEX is too general. I did not change it to avoid undesired side effects. But I added a test in processSingleInterpolationInString to ensure that there is only one match between the quotes.
I also added a test that catch the misbehaviour.
Sorry for the inconvenience.