-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support full reconstruction of HCL from output dictionary #177
Support full reconstruction of HCL from output dictionary #177
Conversation
…o match Terraform syntax instead of Python (fixes amplify-education#172)
(breaks some reconstruction tests, tackling that next)
…ons to make rule-aware
a28991c
to
8652de4
Compare
OK, I think this is ready for a proper review. Since the draft, I added:
|
@kkozik-amplify - still reviewing this? any questions I can answer about my changes? |
@weaversam8 Yeah, need some more time. In the meantime - could you look into the errors that Codacy Analysis PR check reported? |
Hi! While trying to use the code in this branch I found that when parsing the following file:
The program gets stuck in a regex match. I am almost sure it is related to the changes to better support string interpolation (that's why I'm using this branch). |
hcl2/hcl2.lark
Outdated
STRING_LIT : "\"" (STRING_CHARS)* "\"" | ||
STRING_CHARS : /(?:(?!\${)([^"\\]|\\.))+/+ // any character except '"' unless inside a interpolation string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STRING_LIT : "\"" (STRING_CHARS)* "\"" | |
STRING_CHARS : /(?:(?!\${)([^"\\]|\\.))+/+ // any character except '"' unless inside a interpolation string | |
STRING_LIT : "\"" STRING_CHARS? "\"" | |
STRING_CHARS : /(?:(?!\${)([^"\\]|\\.))+/ // any character except '"' |
This seems to fix the problem, but I did not run the tests to check it. It does parse this file correctly tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nfsaavedra Hey! Thanks for the suggestion. Since it's been some time already, I added your fix to the PR myself, all tests are passing.
@weaversam8 any plans when this will be merged? |
2c6d36a
to
d9639af
Compare
a367ac2
to
993ef6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding of the code, LGTM. Do the new tests consider the case where the previous version of the program would get stuck as described here? It's important to avoid regressions. If so, then I have nothing to add :)
880ad1c
to
9e49e8a
Compare
@@ -1,3 +1,3 @@ | |||
locals { | |||
embedded_interpolation = "${module.special_constants.aws_accounts["aaa-${local.foo}-${local.bar}"]}/us-west-2/key_foo" | |||
embedded_interpolation = "(long substring without interpolation); ${module.special_constants.aws_accounts["aaa-${local.foo}-${local.bar}"]}/us-west-2/key_foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nfsaavedra I updated this test case to take issue you found into consideration.
It seems like the original regex would take longer to match non-interpolated string with each character, to the point where for 20 characters it would basically get stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! :)
This is a follow up to #169, to more fully address the use case discussed in #23.
Along the way, I also found and fixed #171, #172, and #173. (To view those changes in isolation, please look at the diffs of the commits linked to those issues specifically.)
I also believe that this supersedes @zabacad's PR #143 to support nested interpolation. (Sorry Ian! I didn't see your PR and recreated your work - but we landed on similar approaches!)
I'm submitting this PR as a draft right now, because while this is technically working, there are a few more things I'd like to add. Namely:
This PR adds:
reverse_transform(hcl_dict)
which returns anAST
generated from a Python dictionaryDictTransformer
class to ensure it generates valid HCL expressions. (See HCL interpolations within dict output contain single quotes #172, I hope it's OK to include this since we just did a major version bump.)hcl2.reconstructor
changes:HCLReverseTransformer
class which aims to reverse the process thathcl2.transformer.DictTransformer
takes (the bread and butter of this PR)test_load
to print the full diff when a file does not match its expected JSONtest_reconstruct_dict
to ensure that the HCL reconstructed from a Python dictionary is syntactically equivalent (whitespace checks will be added to this file soon.)Still a draft PR, as described above, but open to hear any comments or suggestions!