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

hclwrite: Allow blank quoted string block labels #422

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

alisdair
Copy link
Member

@alisdair alisdair commented Nov 18, 2020

The hclsyntax package permits block labels to be blank quoted strings, and defers to the application to determine if this is valid or not. This commit updates hclwrite to allow the same behaviour.

This is in response to an upstream bug report on Terraform, which uses hclwrite to implement its fmt subcommand. Given a block with a blank quoted string label, it currently deletes the label when formatting. This is inconsistent with Terraform's behaviour when parsing HCL, which treats empty labels differently from missing labels:

provider "" {}

provider {}
$ terraform init
There are some problems with the configuration, described below.

The Terraform configuration must be valid before initialization so that
Terraform can determine which modules and providers need to be installed.

Error: Invalid provider local name

  on main.tf line 1:
   1: provider "" {}

 is an invalid provider local name: must have at least one character


Error: Missing name for provider

  on main.tf line 3, in provider:
   3: provider {}

All provider blocks must have 1 labels (name).

I've confirmed that incorporating this change into Terraform fixes the upstream bug.

The hclsyntax package permits block labels to be blank quoted strings,
and defers to the application to determine if this is valid or not. This
commit updates hclwrite to allow the same behaviour.

This is in response to an upstream bug report on Terraform, which uses
hclwrite to implement its fmt subcommand. Given a block with a blank
quoted string label, it currently deletes the label when formatting.
This is inconsistent with Terraform's behaviour when parsing HCL, which
treats empty labels differently from missing labels.
@alisdair alisdair requested a review from a team November 18, 2020 14:27
@alisdair alisdair self-assigned this Nov 18, 2020
@@ -414,7 +421,7 @@ func TestBlockSetLabels(t *testing.T) {
{
`foo "hoge" /* foo */ "" {}`,
"foo",
[]string{"hoge"},
[]string{"hoge", ""},
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this test is covering the parsing of foo "hoge" "" {} as foo "hoge" {}, but it is an example of the bug we are trying to fix with this change, so I've updated the test. @minamijoyo, do you have any concerns about this test update?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisdair I'm not sure why 🤔
I checked the commit history and found that the test case was added by @apparentlymart
7e5b148

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 Sorry, my mistake! I misread the commit history. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

Copy link
Member

Choose a reason for hiding this comment

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

Hmm sorry about that! I imagine I must've misread the input while I was updating this (empty quotes in that position are unusual!) and managed to accidentally write a test that incorrectly passed, rather than a test that caught the bug. 😖

My main goal here was to test the handling of that comment in the middle, so I expect I was focused on it having removed that and not paying attention to how it handled the labels.

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me - I always favor consistency - but it might be a good idea to check in with @apparentlymart and see if there's a specific reason fmt handles it differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants