-
Notifications
You must be signed in to change notification settings - Fork 21
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
Added json unmarshal for potentially encoded JSON string values #95
Conversation
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Need to add a couple more json encoding scenarios to the tests to validate |
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Hey @sgettys , I added below test in the test-cli.sh and the test failed. It looks like we need to always parse the output from terraform -json through a json parser. Do you mind to update the code to handle it?
In addition, we would like to have all PRs to link to the issue that's trying to fix and have an explanation of the change. If the issue is not clear, it's best to summarize the issue in the PR description so that reviewers can easily catch up |
@@ -18,3 +18,7 @@ output "boolean_var" { | |||
output "number_var" { | |||
value = var.number_var | |||
} | |||
|
|||
output "json_encode_html" { | |||
value = "hello&world?@test#$><" |
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.
Can we follow the existing pattern for the other test cases, defining the value as a variable. Also it seems like the variables value are not currently used in the test-cli.sh. It feels confusing that's not used. If we are going to define it, I think we should use it in our tests.
Great test case, thanks for finding that! I'll address that test case and update the PR, thanks! |
Might need to think about how to solve more complex nested json encoded values. Maybe should define some behaviors for this. Theoretically we could have n-nested json objects as outputs from terraform that could potentially have encoded JSON at any level. Let me dig into this a little more, might have to do a custom unmarshaler to solve this. |
…ight have escaped html Signed-off-by: Steven Gettys <s.gettys@f5.com>
Closes #94 |
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Reworked to support decoding nested values by unmarshaling into empty interface and then re-encoding with a custom json encoder that skips the html encoding. |
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.
Thanks for the quick update. I left some questions on the PR but overall it looks good to me
pkg/terraform/terraform.go
Outdated
// string so that the outer quotes are stripped, otherwise return the raw | ||
// json syntax directly. | ||
var outString string | ||
if err = json.Unmarshal(outJSON, &outString); err == nil { |
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.
Do we still need to unmarshal the entire output json in order to remove the quotes here?
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.
I could also do type assertion and skip then re-encode for string types then we wouldn't need this second unmarshal
Signed-off-by: Steven Gettys <s.gettys@f5.com>
} | ||
// For all other JSON types we'll return the raw JSON syntax directly. | ||
return outJSON, nil | ||
return bytes.TrimRight(buffer.Bytes(), "\n"), nil |
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.
Does this mean we don't need to trim the new line character when the output is a 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.
No since we're not re-encoding it. The json.Encode is what puts the newline on the end:
From the docs - Encode writes the JSON encoding of v to the stream, followed by a newline character.
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 this comment, it looks like the original code is for removing it from Terraform output?
https://github.com/getporter/terraform-mixin/pull/95/files#diff-282757e263fe02ec96e35fd1120b9263db1ca315d75cea25d3ccf68239bd7472L59
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.
I did some testing. It looks like json.Unmarshal()
removes the new line character. We should be good. Thanks for answering my questions :)
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.
Yep that's what I was in the middle of validating! Beat me to it :)
Signed-off-by: Steven Gettys s.gettys@f5.com
Closes #94
Terraform outputs from porter included escaped html characters for things like connection strings. This is a regression due to switching from -raw to -json. The reason the switch occurred is because -raw only supports number, boolean, and string outputs from terraform.
The mixin will decode all of the json strings and remove the html escapes. This works for complex nested objects like arrays and maps as well as strings