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/fmt: Ensure all variable files ending in .pkrvars.hcl get formatted #10377

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

nywilken
Copy link
Contributor

Before change

⇶  packer fmt -check /tmp/unformatted.pkrvars.hcl
Error: Cannot tell whether /tmp/unformatted.pkrvars.hcl contains HCL2 configuration data

⇶  echo $?
1

After fix

⇶  packer fmt -check /tmp/unformatted.pkrvars.hcl
/tmp/unformatted.pkrvars.hcl

⇶  echo $?
3

⇶  packer fmt -check command/test-fixtures/fmt
command/test-fixtures/fmt/unformatted.pkr.hcl
command/test-fixtures/fmt/unformatted.auto.pkrvars.hcl
command/test-fixtures/fmt/unformatted.pkrvars.hcl

Closes #10350

…ormatted

Before change
```
⇶  packer fmt -check /tmp/unformatted.pkrvars.hcl
Error: Cannot tell whether /tmp/unformatted.pkrvars.hcl contains HCL2 configuration data

⇶  echo $?
1
```

After fix
```
⇶  packer fmt -check /tmp/unformatted.pkrvars.hcl
/tmp/unformatted.pkrvars.hcl

⇶  echo $?
3

⇶  packer fmt -check command/test-fixtures/fmt
command/test-fixtures/fmt/unformatted.pkr.hcl
command/test-fixtures/fmt/unformatted.auto.pkrvars.hcl
command/test-fixtures/fmt/unformatted.pkrvars.hcl

```
@nywilken nywilken requested a review from a team as a code owner December 11, 2020 16:04
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #10377 (ec18450) into master (e89db37) will increase coverage by 0.03%.
The diff coverage is 50.00%.

Impacted Files Coverage Δ
command/fmt.go 37.33% <0.00%> (+37.33%) ⬆️
hcl2template/parser.go 69.51% <100.00%> (ø)
builder/azure/arm/tempname.go 80.55% <0.00%> (-5.56%) ⬇️
packer-plugin-sdk/packer/communicator.go 76.59% <0.00%> (+2.12%) ⬆️
command/cli.go 76.56% <0.00%> (+9.37%) ⬆️

@@ -162,7 +164,7 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st

// parse var files
{
hclVarFiles, jsonVarFiles, moreDiags := GetHCL2Files(filename, hcl2VarFileExt, hcl2VarJsonFileExt)
hclVarFiles, jsonVarFiles, moreDiags := GetHCL2Files(filename, hcl2AutoVarFileExt, hcl2AutoVarJsonFileExt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, super good catch !

Copy link
Contributor

@azr azr Dec 14, 2020

Choose a reason for hiding this comment

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

@nywilken I was referring to this change here: before this the HCL2 parse would read all .varvars.hcl files in the folder, now it only reads .auto.pkrvars.hcl files ( which I think is a good way to go ).

Copy link
Contributor

Choose a reason for hiding this comment

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

So if anyone was mistakenly relying on this behaviour. Bang ! I say mistakenly because the docs says that only .auto files in the same folder will be auto parsed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks. To confirm you mean .auto.pkrvars.hcl , right? If so then the behavior was correct to begin with. I just renamed the variable to so that I could use hcl2VarFileExt for the non-auto variable file extension 😄 I'm going to merge since this this is what we want. But do let me know if I misunderstood the case for .pkr.hcl files.

Copy link
Contributor

Choose a reason for hiding this comment

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

OOoohh right I misread the diff sorry ! Yup even better !

Copy link
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

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

Super good one, LGTM. I hadn't realised that we were reading all .pkr.hcl files from the same project folder ( unlike the docs stated ) that was def a bug. I hope the breaking change won't be too annoying.

fatalCommand(t, c.Meta)
}
expected := ""
assert.Equal(t, expected, strings.TrimSpace(s.String()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally for another PR but one nice thing we could have done is having a input file and an expected per folder, making the tests easy to read & add.

@nywilken
Copy link
Contributor Author

I hadn't realised that we were reading all .pkr.hcl files from the same project folder ( unlike the docs stated ) that was def a bug. I hope the breaking change won't be too annoying.

@azr is this in reference to the the fmt command or some other command? I'm not sure what the breaking change you are referring to is here.

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

Nice! LGTM 👍🏼

@sylviamoss sylviamoss added core Core components of Packer hcl2 labels Dec 14, 2020
@nywilken nywilken merged commit 4e58987 into master Dec 14, 2020
@nywilken nywilken deleted the fix-fmt-cmd branch December 14, 2020 15:30
@ghost
Copy link

ghost commented Jan 14, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core Core components of Packer hcl2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to fmt .pkrvars.hcl extension
3 participants