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

Testing Migration (from SDKv2) of Datasources does not fail when fields are missing #875

Closed
apricote opened this issue Nov 21, 2023 · 4 comments · Fixed by #877
Closed
Assignees
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@apricote
Copy link

Module version

v1.4.2

Relevant provider source code

We are currently in the process of migration our provider to the plugin-framework. To test that we included every field in the new data source implementation, we wrote a test similar to the docs at: https://developer.hashicorp.com/terraform/plugin/framework/migrating/testing#example

func TestAccHcloudDataSourceDatacenters_UpgradePluginFramework(t *testing.T) {
	tmplMan := testtemplate.Manager{}

	datacentersD := &datacenter.DDataList{}
	datacentersD.SetRName("ds")

	resource.ParallelTest(t, resource.TestCase{
		PreCheck: e2etests.PreCheck(t),
		Steps: []resource.TestStep{
			{
				ExternalProviders: map[string]resource.ExternalProvider{
					"hcloud": {
                                                # Last version before migrating this specific data source
						VersionConstraint: "1.44.1",
						Source:            "hetznercloud/hcloud",
					}
				},

				Config: tmplMan.Render(t,
					"testdata/d/hcloud_datacenters", datacentersD,
				),
			},
			{
				ProtoV6ProviderFactories: e2etests.ProtoV6ProviderFactories(),

				Config: tmplMan.Render(t,
					"testdata/d/hcloud_datacenters", datacentersD,
				),

				PlanOnly: true,
			},
		},
	})
}

Terraform Configuration Files

Both test steps use the same configuration (templated):

data "hcloud_datacenters" "ds" {}

Debug Output

Expected Behavior

We would expect any missing or different fields between the two steps to cause the test to fail. We need this behaviour to test that we added all fields with the same values as the previous implementation. Especially as this is the documented/recommended way to test the migration.

Actual Behavior

The test does not fail if we remove a field in the code in the new plugin-framework implementation.

We tested this for Resources and that works as expected, only Datasources are allowed to have fields missing.

Steps to Reproduce

Not sure if there is an easy way to reproduce this. If wanted, I can push a branch to our repository that demonstrates the issue.

References

We noticed this while testing hetznercloud/terraform-provider-hcloud#779

We ended up using a null_resource that gets the datasource jsonencoded in a trigger to work around the issue. Would be nice if we can drop this in the future.

@apricote apricote added the bug Something isn't working label Nov 21, 2023
@bflad bflad added documentation Improvements or additions to documentation and removed bug Something isn't working labels Nov 21, 2023
@bflad bflad self-assigned this Nov 21, 2023
@bflad
Copy link
Contributor

bflad commented Nov 21, 2023

Hi @apricote 👋 Thank you for raising this and sorry for the misleading documentation.

This style of migration testing of data sources might have been written when Terraform still sometimes referenced prior state for data sources during planning, which I believe was fully removed somewhere around Terraform 1.3. While this migration testing pattern is no longer directly applicable for data sources as written, it is still valid for managed resources. We will certainly update the documentation to reflect that.

In terms of actually performing the testing you are trying to accomplish, it sounds like you found a decent workaround. Using a managed resource, such as null_resource or terraform_data, that stores a value in state and would show a change during plan can be tested using PlanOnly or plan check functionality. It may also be possible to verify this via an output value too, although there may not be a pre-made plan check available to cover testing "this output value did not change". We will investigate this side of things as well to document the pattern and ensure the same level of confidence when migrating data sources as managed resources.

Thanks again.

@bflad
Copy link
Contributor

bflad commented Nov 21, 2023

One awkward detail that wasn't previously covered by the migration testing documentation was also that this sort of "full" data source value checking will eventually cause test failures later on when new attributes are expectedly added to the framework implementation of the data source, meaning that developers will either need to remove the test or pin the second test step to the last compatible framework provider version. #877 includes a "full" data source value checking example, however it does recommend individual attribute checks. Most nuances between SDK and framework implementations of data sources can be captured appropriately this way.

Aside: The same is also generally true for managed resources as well, but there is also the recommendation to not test attribute values that are explicitly configured. Since this migration testing is generally for finding situations where the SDK implementations stored values in state differently than they were configured, which is explicitly not allowed with framework implementations, it seems better to continue recommending the migration tests rather than muddy the documentation too much in this regard.

@apricote
Copy link
Author

Thank you very much @bflad for the quick response and docs improvements!

We are updating our tests to match the new docs 🎆 hetznercloud/terraform-provider-hcloud#797

this sort of "full" data source value checking will eventually cause test failures later on when new attributes are expectedly added to the framework implementation of the data source

We have considered this, and plan on removing the test step when this happens (per resource) or when we are fully done with the migration. Just for the individual migration PRs its really nice to know that the resources match exactly.

jooola added a commit to hetznercloud/terraform-provider-hcloud that referenced this issue Nov 22, 2023
jooola added a commit to hetznercloud/terraform-provider-hcloud that referenced this issue Nov 22, 2023
As suggested in
hashicorp/terraform-plugin-framework#875 (comment)

---------

Co-authored-by: Julian Tölle <julian.toelle@hetzner-cloud.de>
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants