-
Notifications
You must be signed in to change notification settings - Fork 17
importstate: Expect an error when plannable import is not supported #460
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
Conversation
This raises a question: what next? What are we asking a test author to do with this information? If the answer is "use Terraform 1.5.0+ in the test," I'm realizing this is harder than it sounds. Here is a reasonable expectation, in my opinion: Let's say that my CI runs tests against multiple Terraform versions. For simplicity: 1.1, 1.2, 1.3, 1.4, and 1.5. I have an existing test in the familiar pattern of: [1] Config, [2] Import Command. I want to be able to change my existing test to: [1] Config, [2] Import Command, [3] Import Block. And I want that test to pass the whole matrix. Could I opt in to "skip import blocks if not supported?" Is that a missing feature that we want? I think it is 😃 Edit for clarity: this question does not block review & merge of this pull request. |
austinvalle
left a comment
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, just had two comments
This raises a question: what next? What are we asking a test author to do with this information? If the answer is "use Terraform 1.5.0+ in the test," I'm realizing this is harder than it sounds.
Here is a reasonable expectation, in my opinion:
Let's say that my CI runs tests against multiple Terraform versions. For simplicity: 1.1, 1.2, 1.3, 1.4, and 1.5.
I have an existing test in the familiar pattern of: [1] Config, [2] Import Command.
I want to be able to change my existing test to: [1] Config, [2] Import Command, [3] Import Block. And I want that test to pass the whole matrix.
Could I opt in to "skip import blocks if not supported?" Is that a missing feature that we want? I think it is 😃
My two cents are, because it's ambiguous what we should suggest to the user, we should describe the two likely options in the error message. Hypothetical message could be:
ImportState steps using plannable import blocks require Terraform 1.5.0 or later. Either
upgrade the Terraform version running the test or add a `TerraformVersionChecks` to
the test case to skip this test.
https://developer.hashicorp.com/terraform/plugin/testing/acceptance-tests/tfversion-checks#skip-version-checks
I'm not 100% sure, but it seems unlikely that provider developers are running real acceptance test suites against all versions of Terraform like we do with utility providers. Even if they are, I wouldn't expect them to run tests on the import CLI command and import blocks in the same test case, since at that point they're really just testing Terraform itself. (they are both being imported by the id string, so really they aren't wildly different test paths for the provider like ImportBlockWithIdentity will be)
Contents of this change:
Refactoring: move
testing_new_import_block_id_test.goto a newimportstatepackage. This simplifies naming (import_block_with_id_test.go) and paves a path for more refactoring as we go.Refactoring: de-duplicate
examplecloudtest provider setup by extracting intoexamplecloud_test.go.Add a new test:
TestTest_TestStep_ImportBlockId_FailWhenPlannableImportIsNotSupportedexpects a helpful error when using a Terraform version that does not support plannable import.Before:
After: