Fix: run terraform validate on modules again
#20230
Conversation
We don't depend on the backend config, so we don't actually need to target a terraform_deployment
everything still worked because we can technically still use terraform_deployment targets, they just get filtered out
lilatomic
added
bug
category:bugfix
|
We'll probably want to backport this to 2.18 and 2.19 too |
|
I've marked it for 2.18. (And also, leaving a comment so I get notified when this merges, so I can cut 2.18.1rc2 immediately, to potentially get 2.18.1 out soon.) |
| --- | ||
| > 🚧 Terraform support is in alpha stage | ||
| > | ||
| > Pants is currently building support for developing and deploying Terraform. Simple usecases might be supported, but many options are missing. |
| @@ -0,0 +1,124 @@ | |||
| --- | |||
There was a problem hiding this comment.
I'm hoping that we get some feedback once people can see that it exists haha
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class TerraformValidateFieldSet(TerraformFieldSet): |
There was a problem hiding this comment.
Doesn't this need to extend the parent's required_fields to include SkipTerraformValidateField?
There was a problem hiding this comment.
I didn't know the answer, so I dug around. Seems the typical way does not add the skip field to the required fields, but also doesn't inherit from the parent FieldSet. Usually the fieldset has just the sources field as required. I think that's to let them cover all the target types that have those sources (for example, isort needs to cover python_source and python_test). The Helm backend, OTOH, subclasses. I think that allows us to avoid duplicating all the fields. I think it makes sense for us to follow the Helm backend, since Terraform files are mostly single-purpose. We might want to revisit that if we decide we want the check both modules and deployments (like helm.kubeconform).
I think using tgt.get is what allows the field to be optional and fill in the default.
lilatomic
force-pushed
the
terraform/20222-skip-validate
branch
from
bc675a1
to
3db5912
Compare
Co-authored-by: Benjy Weinberger <[email protected]>
| @@ -0,0 +1,50 @@ | |||
| --- | |||
There was a problem hiding this comment.
Maybe add this documentation page in a separate PR in order to keep this PR on one topic?
There was a problem hiding this comment.
@benjyw: thoughts on above question?
There was a problem hiding this comment.
I'm neutral on that question, since the docs are pertinent to the code change, even if wider.
Only running `terraform validate` on deployments is unexpected for users. See #20222 . Running `terraform validate` on all modules might lead to errors with Terraform, but those are predictable and understandable by users. This MR: - Runs `terraform validate` on `terraform_modules` again - Adds the `skip_terraform_validate` field for `terraform_modules` which cannot be `validate`d by Terraform - Adds some documentation for the Terraform backend It leaves open the possibility of validating both `terraform_deployment`s and `terraform_module`s. Hopefully we can work out something like the Helm backend's handling of kubeconform. --------- Co-authored-by: Benjy Weinberger <[email protected]>
Only running `terraform validate` on deployments is unexpected for users. See #20222 . Running `terraform validate` on all modules might lead to errors with Terraform, but those are predictable and understandable by users. This MR: - Runs `terraform validate` on `terraform_modules` again - Adds the `skip_terraform_validate` field for `terraform_modules` which cannot be `validate`d by Terraform - Adds some documentation for the Terraform backend It leaves open the possibility of validating both `terraform_deployment`s and `terraform_module`s. Hopefully we can work out something like the Helm backend's handling of kubeconform. --------- Co-authored-by: Benjy Weinberger <[email protected]>
#20299) Only running `terraform validate` on deployments is unexpected for users. See #20222 . Running `terraform validate` on all modules might lead to errors with Terraform, but those are predictable and understandable by users. This MR: - Runs `terraform validate` on `terraform_modules` again - Adds the `skip_terraform_validate` field for `terraform_modules` which cannot be `validate`d by Terraform - Adds some documentation for the Terraform backend It leaves open the possibility of validating both `terraform_deployment`s and `terraform_module`s. Hopefully we can work out something like the Helm backend's handling of kubeconform. Co-authored-by: Daniel Goldman <[email protected]>
#20298) Only running `terraform validate` on deployments is unexpected for users. See #20222 . Running `terraform validate` on all modules might lead to errors with Terraform, but those are predictable and understandable by users. This MR: - Runs `terraform validate` on `terraform_modules` again - Adds the `skip_terraform_validate` field for `terraform_modules` which cannot be `validate`d by Terraform - Adds some documentation for the Terraform backend It leaves open the possibility of validating both `terraform_deployment`s and `terraform_module`s. Hopefully we can work out something like the Helm backend's handling of kubeconform. Co-authored-by: Daniel Goldman <[email protected]>
Only running
terraform validateon deployments is unexpected for users. See #20222 . Runningterraform validateon all modules might lead to errors with Terraform, but those are predictable and understandable by users.This MR:
terraform validateonterraform_modulesagainskip_terraform_validatefield forterraform_moduleswhich cannot bevalidated by TerraformIt leaves open the possibility of validating both
terraform_deployments andterraform_modules. Hopefully we can work out something like the Helm backend's handling of kubeconform.