Skip to content
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

command/format: Correctly quote diff object keys #30766

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Mar 30, 2022

This is a port of #25443 to the new diff renderer.

Fixes #21749

When rendering a diff, we should quote object attribute names if the string representation is not a valid identifier. While this is not
strictly necessary, it makes the diff output more closely resemble the configuration language, which is less confusing.

This commit applies to both top-level schema attributes and any object value attributes. We use a simplistic "%q" Go format string to quote the strings, which is not strictly identical to HCL's quoting requirements,but is the pattern used elsewhere in HCL and Terraform.

When rendering a diff, we should quote object attribute names if the
string representation is not a valid identifier. While this is not
strictly necessary, it makes the diff output more closely resemble the
configuration language, which is less confusing.

This commit applies to both top-level schema attributes and any object
value attributes. We use a simplistic "%q" Go format string to quote the
strings, which is not strictly identical to HCL's quoting requirements,
but is the pattern used elsewhere in HCL and Terraform.

Co-Authored-By: Katy Moe <[email protected]>
@kmoe kmoe requested a review from a team March 30, 2022 14:49
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@kmoe kmoe added the 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Apr 1, 2022
@kmoe kmoe merged commit 5907a86 into main Apr 1, 2022
@kmoe kmoe deleted the kmoe/diff-renderer-invalid-identifier branch April 1, 2022 09:09
@github-actions
Copy link

github-actions bot commented Apr 1, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@stevehipwell
Copy link

This PR seems to break the representation of map(type) where the keys have been provided as string to allow special characters.

      ~ tags                  = {
          + "\"test.io/foo\"" = "my"
          + "\"test.io/bar\"" = "test"
            # (8 unchanged elements hidden)
        }

@alisdair
Copy link
Contributor

@stevehipwell Thanks for the note!

If those two keys in your example are strings containing escaped quotes, then this behaviour is correct. This patch is intended to make the diff output more closely represent the HCL representation of a value. Writing the already-quoted value "test.io/foo" as a string literal in HCL results in "\"test.io/foo\"".

If that behaviour is not what you're describing, I'm unable to reproduce the problem, so filing a separate issue would help us investigate.

@stevehipwell
Copy link

@alisdair sorry I forgot the input values, they were as follows and didn't have any escaped quotes. They were also passing through a variable typed as map(string) so there is no type confusion.

locals {
  tags = {
    "test.io/foo" = "my"
    "test.io/bar" = "test"
  }
}

You can replicate this behaviour by using the following simple object to set the tags on an AWS resource (or other resource accepting map(string)) and seeing how the diff is represented.

locals {
  tags = {
    foo           = "my"
    "bar"        = "test"
    "foo/bar" = "my-test"
  }
}

Based on this test we can see that map keys with characters that HCL doesn't support in keys are being treated differently to keys without special character and that it's not the wrapping of the key with " that causes the issue here. Please note that these aren't special string characters. To me this representation looks completely wrong and seems like the HCL implementation is leaking out into user space.

As map(string) is HCL syntax on top of the Go map[string]string and the no-quote pattern is syntax sugar which can be used when there aren't any special characters, but still results in a quoted value in the diff, I'm struggling to see why the diff now shows extra wrapping \" characters when a key has certain characters in it?

@alisdair
Copy link
Contributor

@stevehipwell After some more poking around I found a reproduction. It seems like this only happens on update, and not on create, which is why I missed it before. Tracking in #30854, thanks for the report!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terraform plan produces invalid hcl when there are colons in property identifiers
3 participants