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

Terraform validate not applied with check goal #20222

Closed
xaniasd opened this issue Nov 21, 2023 · 9 comments
Closed

Terraform validate not applied with check goal #20222

xaniasd opened this issue Nov 21, 2023 · 9 comments
Labels
backend: Terraform Terraform backend-related issues bug
Milestone

Comments

@xaniasd
Copy link

xaniasd commented Nov 21, 2023

Describe the bug
A clear and concise description of the bug.
when running the check goal on terraform modules, the command executes successfully (with exit code 0) but does not perform any actual validation. Additionally, no logs are shown even at debug log level. Version v2.17.0 works as expected.

Pants version
Which version of Pants are you using?
v2.18.0

OS
Are you encountering the bug on MacOS, Linux, or both?
MacOS

Additional info
Add any other information about the problem here, such as attachments or links to gists, if relevant.
This test repository can be used to reproduce the problem. Goals fmt and lint work as expected, check on the other hand not.

@xaniasd xaniasd added the bug label Nov 21, 2023
@huonw huonw added the backend: Terraform Terraform backend-related issues label Nov 21, 2023
@huonw huonw added this to the 2.18.x milestone Nov 21, 2023
@huonw
Copy link
Contributor

huonw commented Nov 21, 2023

@lilatomic this appears to be a regression from 2.17 to 2.18, which suggests it might be related to #18974 . Do you happen to have insight into what's going on here?

@lilatomic
Copy link
Contributor

ah, right, this is my bad for not writing up a doc on Terraform.
TLDR: you need to add a terraform_deployment target because terraform validate can only run safely against "root" modules (modules designed to be deployed)

terraform_deployment(name="deployment", root_module="terraform/modules/test:test")

The change is #19185 , comment #19185 (comment) .
The challenge is something upstream in Terraform itself hashicorp/terraform#28490. It is possible to lint any Terraform file, so we can lint all terraform_modules. But terraform validate cannot be run on just any collection of Terraform files (not even complete modules), because it requires providers to be present. But providers are only guaranteed to be present on a "root" module, leading to the situation in that issue.


I know this isn't ideal and is definitely unexpected. I thought this was the best position to take, since it won't cause errors caused by Terraform's behaviour. I've also found this to be a reasonable position with my own work, where all modules are consumed by deployments and checking modules was causing errors. I'm open to suggestions on what would be better! Would it be better to attempt to check all modules and offer an opt-out toggle? Would a doc explaining this behaviour be sufficient?

It's entirely on me that the doc on the Terraform backend and porting instructions isn't written. I'll have time tomorrow to write it up.

@xaniasd
Copy link
Author

xaniasd commented Nov 22, 2023

Hi @lilatomic, thanks for the response. Thanks for putting some (needed) work on the terraform backend, even without the docs :)

On the overall approach, doesn't marking a module as deployment imply the risk of actually deploying it with experimental-deploy? We'd need a skip_deploy flag to mitigate this. Alternatively I'd go with a skip_check flag in the terraform_module itself and let users decide whether a module can be validated or not.

As a sidenote, adding the line you suggest actually caused an error

09:39:37.89 [ERROR] 1 Exception encountered:

Engine traceback:
  in `check` goal

OptionsError: You must explicitly specify the default Python interpreter versions your code is intended to run against.

You specify these interpreter constraints using the `interpreter_constraints` option in the `[python]` section of pants.toml.

We recommend constraining to a single interpreter minor version if you can, e.g., `interpreter_constraints = ['==3.11.*']`, or at least a small number of interpreter minor versions, e.g., `interpreter_constraints = ['>=3.10,<3.12']`.

Individual targets can override these default interpreter constraints, if different parts of your codebase run against different python interpreter versions in a single repo.

See https://www.pantsbuild.org/v2.18/docs/python-interpreter-compatibility for details.

I'm not using a python backend in this example, so this is rather confusing IMHO. Adding the config solves the issue, perhaps worth mentioning in the docs?

@lilatomic
Copy link
Contributor

I think the interpreter constraints is an internal thing Pants needs to run the dependency parser. We can definitely call that out in the docs.
Yeah, you're right that adding a deployment would make it liable to being deployed. And terraform_deployment(..., skip_deploy=True) seems a little silly to me. Adding a skip_terraform_validate field is on the todo list anyhow and simple enough to add; and it gives maximum control to the end user.
Can you also talk a bit about how you'd like to use the Terraform backend? At work I've got modules and deployments in the same repo, and use multiple aliased providers. For me, validateing modules was a bug, and validateing on a deployment is the correct solution from that thread. Do you have modules that you publish to a registry?

@xaniasd
Copy link
Author

xaniasd commented Nov 22, 2023

Sort of. We have one repo that holds the terraform modules and another that generates tfvars for these modules, we basically generate instances of the "root" modules dynamically. So development and deployment of said modules happen in separate repositories, we won't use pants for deployment at least for the foreseeable future.

@lilatomic
Copy link
Contributor

Thanks for explaining, I'll keep that in mind as I'm developing. I've got an MR up that will validate modules again, adds the skip_terraform_validate flag, and also adds some documentation. I'd appreciate if you could review it.

@xaniasd
Copy link
Author

xaniasd commented Nov 24, 2023

as far as my limited knowledge of pants internals goes, it looks good to me! Thanks for the quick fix.

btw, it is often useful to determine the order root modules are applied, I think it should be possible to use pants to our advantage for this (e.g. with terraform_deployment dependencies?).

tdyas pushed a commit that referenced this issue Dec 16, 2023
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 <benjyw@gmail.com>
WorkerPants pushed a commit that referenced this issue Dec 16, 2023
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 <benjyw@gmail.com>
WorkerPants pushed a commit that referenced this issue Dec 16, 2023
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 <benjyw@gmail.com>
benjyw pushed a commit that referenced this issue Dec 17, 2023
#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 <merkavabuilder@gmail.com>
benjyw pushed a commit that referenced this issue Dec 17, 2023
#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 <merkavabuilder@gmail.com>
@lilatomic
Copy link
Contributor

Sorry, I missed the last query there. I don't think Pants currently has a way to set the deployment order of deployable targets. I think using the dependencies field here doesn't work because the dependency is placed on the files of the terraform_deployment, not on the result of the deployment.
It is possible to implement that, though, just needs to get done.

@huonw huonw modified the milestones: 2.18.x, 2.19.x Mar 4, 2024
@huonw huonw modified the milestones: 2.19.x, 2.20.x Jun 4, 2024
@lilatomic
Copy link
Contributor

Hey, we've solved this issue in mainline now, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Terraform Terraform backend-related issues bug
Projects
None yet
Development

No branches or pull requests

3 participants