-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[HPR-837] Update validation options for undeclared variables #12104
Conversation
92bf805
to
9f1dfc4
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.
Overall LGTM, I do have some concerns relative to the naming of both the option and in the code, but the code looks good to me.
"variable %[1]q {\n"+ | ||
" type = string\n"+ | ||
" type = string\n"+ | ||
" default = null\n"+ |
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.
More a question than anything, is null
a valid default value for 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.
Yes, this does work. We could set the default value to an empty string but that may not immediately imply to the user that the variable is optional. We have the use of default = null
documented as the preferred way to make a value of any type optional. So I went with that.
I've considered adding a comment to the example block to indicate that both type and the default value should be changed to reflect the actual value in the var-file. But that might be too much info. Thoughts?
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 I think you're right, it may be a bit much, just stating they need to declare a variable block to make their template valid is good enough imo, that was just me asking a question out of curiosity, I didn't know that null
was accepted for any type and was the preferred way to make a variable optional.
If that's documented and known, no issue in proposing null
to be the default value, users can choose whatever else suits their need should they want to.
Besides it's just an example of what they need to add to their template, they'll have to change its contents 90% of the time because the default value is not required, different, or because the type is not string, so I don't think we need to worry too much about what's in there.
cc5c185
to
988218c
Compare
I've re-rolled to account for the provided feedback. Feel free to review the updates when ready. |
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.
Just a last thing I left a comment for, other than that, LGTM!
"variable %[1]q {\n"+ | ||
" type = string\n"+ | ||
" type = string\n"+ | ||
" default = null\n"+ |
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 I think you're right, it may be a bit much, just stating they need to declare a variable block to make their template valid is good enough imo, that was just me asking a question out of curiosity, I didn't know that null
was accepted for any type and was the preferred way to make a variable optional.
If that's documented and known, no issue in proposing null
to be the default value, users can choose whatever else suits their need should they want to.
Besides it's just an example of what they need to add to their template, they'll have to change its contents 90% of the time because the default value is not required, different, or because the type is not string, so I don't think we need to worry too much about what's in there.
37a119c
to
ae73139
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.
LGTM
In an effort to help users move from JSON to HCL2 templates the support for variable definitions files are being updated to ignore undeclared variable warnings on build execution. For legacy JSON templates builds no warnings are displayed when var-files contain undeclared variables. Since preferred mode HCL2 templates is to be explicit with variable declarations - they must be declared to be used - validation for undeclared variables still warns when running `packer validate`. A new flag has been added to the validate command that can be used to disable undeclared variable warnings. * Update validation test for unused variables Example Run ``` ~> go run . validate -no-warn-undeclared-var -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl The configuration is valid. ~> go run . validate -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl Warning: Undefined variable The variable "unused" was set but was not declared as an input variable. To declare variable "unused" place this block in one of your .pkr.hcl files, such as variables.pkr.hcl variable "unused" { type = string default = null } The configuration is valid. ~> go run . build -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl file.chocolate: output will be in this color. Build 'file.chocolate' finished after 744 microseconds. ==> Wait completed after 798 microseconds ==> Builds finished. The artifacts of successful builds are: --> file.chocolate: Stored file: chocolate.txt ```
The field name Strict is a bit vague since it is only used for checking against undeclared variables within a var-file definition. To mitigate against potential overloading of this field it is being renamed to be more explicit on its usage.
Now that the default behaviour is to not display warnings for undeclared variables an optional flag has been added to toggle the old behaviour. ``` ~> go run . build -warn-on-undeclared-var -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl Warning: Undefined variable The variable "unused" was set but was not declared as an input variable. To declare variable "unused" place this block in one of your .pkr.hcl files, such as variables.pkr.hcl variable "unused" { type = string default = null } file.chocolate: output will be in this color. Build 'file.chocolate' finished after 762 microseconds. ==> Wait completed after 799 microseconds ==> Builds finished. The artifacts of successful builds are: --> file.chocolate: Stored file: chocolate.txt ```
ae73139
to
7820bd2
Compare
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 issues. |
In an effort to help users move from JSON to HCL2 templates the support for variable definitions files are being updated to ignore undeclared variable warnings on build execution. For legacy JSON template builds no warnings are displayed when var-files contain undeclared variables. So the behavior is sort of the same.
Since the preferred mode for HCL2 templates is to be explicit with variable declarations - they must be declared to be used - validation for undeclared variables still warns when running
packer validate
. A new flag has been added to the validate command that can be used to disable undeclared variable warnings.Closes: #12109
Related to: #9822
Example
packer validate
runExample
packer build
runs