Add terraform block to config when not already present

[bendbennett]

Closes: #215

Dependency: #228

Considerations/Questions

• Currently, there is no handling to add required provider entries into the terraform block if TestStep.Config contains JSON.
  • Does this need implementing?
• The entries in the terraform required providers block are not added prior to the initial call to wd.Init, or in testIDRefresh()
  • Interested to hear comments on whether this is viewed as ok.
• The providerConfigTestCase() removes entries from testCase.Providers, when an entry with the same key is found in testCase.ProviderFactories.
  • This was required to avoid a panic generated by the presence of two entries with the same key in the terraform required providers block when running TestTest_TestCase_Providers (i.e., TestTest_TestCase_Providers results in test appearing as a key in both testCase.Providers, and testCase.ProviderFactories.

[bflad]

Given that we could potentially negatively affect working tests after this feature is added, I would suggest we go ahead and also include an enhancement changelog for this, e.g.

helper/resource: Automatically add `required_providers` configuration to `TestStep.Config` configuration when necessary to support future provider functionality testing

[bflad]

For this to work as expected, the terraform configuration in this case must include a correct required_providers definition to match the provider(s) being tested and included in the configuration, e.g.

terraform {
  required_providers {
    NAME = {
      source = "ADDRESS"
    }
  }
}

This is what future Terraform provider functionality will require in the configuration so Terraform explicitly knows which provider is being referenced in a configuration (rather than implicitly guessing any missing provider declaration is in the hashicorp namespace). Here's some internal background about this problem: https://github.com/hashicorp/terraform-proposals/issues/66

There is somewhat of an example of this in (TestStep).providerConfig() but its only wired to the ExternalProviders information. This implementation needs to account for a lot more, unfortunately:

I believe the NAME values must contain all the map keys of all the following defined on the TestCase/TestStep:

• Providers †
• ProviderFactories †
• ExternalProviders
• ProtoV5ProviderFactories
• ProtoV6ProviderFactories

(† We may not necessarily need to include Providers/ProviderFactories as terraform-plugin-sdk/v2 providers will never receive the future enhancements needing the required provider configuration, however I think to prevent potential bugs where Providers/ProviderFactories and ExternalProviders is being used, we probably should just properly do it for everything.)

And the ADDRESS value may also need to depend on things like the TF_ACC_PROVIDER_NAMESPACE environment variable. We might need to introduce some additional testing with non-hashicorp namespace providers and the ProtoV{5,6}ProviderFactories fields to verify whether that value must be consulted or not.

[bflad]

Overall, I think this is what we want! To answer some of your questions:

Currently, there is no handling to add required provider entries into the terraform block if TestStep.Config contains JSON.
Does this need implementing?

I'd say no for now -- we have treated JSON configuration as a "you know what you're doing", similar to the new configuration file/directory handling.

The entries in the terraform required providers block are not added prior to the initial call to wd.Init, or in testIDRefresh()

I think this is okay. Init only needs external providers (or whatever it currently gets). ID refresh is a very legacy testing feature that doesn't need to be touched. It honestly should be deprecated, but I forget why that hasn't already been done other than maybe previously thinking all the next major version deprecations would be done at once.

---

At some point we'll clean up all those gnarly helper/resource function signatures. 😬 😅

[bflad]

I think we can omit this handling here (at least for now) -- its odd to have empty required_provider configuration and honestly not sure if Terraform will always treat that as valid. 👍

[bflad]

I think we discussed this out of band, but it might be good to set this to 1.0.0 with the reasoning that we'd eventually remove this version check entirely in the terraform-plugin-testing major version we drop protocol version 5 support. Another potentially good reason to do this now is that it won't potentially delay feedback for folks who may be already be testing on 1.x and upgrade their terraform-plugin-testing.

If you could also drop a quick code comment here about why this version check exists at all, that would be fantastic!

[bflad]

Good consideration callout in the PR description about JSON, since we currently treat JSON as "you know what you're doing" and our future functionality testing documentation will always show Terraform configuration language, I think just putting a quick "Terraform language" before the word "configuration" here will suffice. Also if we do update the check to 1.0, that will need to be updated here as well. 😄

[github-actions]

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 contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.