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

upgrade provider to use terraform-plugin-sdk v2 #492

Merged

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Sep 23, 2020

This PR upgrades the provider to use v2 of the terraform-plugin-sdk.

This makes the minimum necessary changes to get the provider to work:

  • The hashcode.String function was made internal to the SDK. The implementation was copied to a new function: SDKHashString

  • terraform.ResourceProvider was removed. Code now uses *schema.Provider directly.

  • schema.CustomizeDiffFunc now takes context.Context parameter.

  • The SetPartial function was removed. I left in the calls to d.Partial but the partial state usage need to be audited for correctness.

  • The Removed attribute for obsolete fields was itself removed. I removed the instance of it in the provider.

Why upgrade to v2? It provides the opportunity to update resources/datasources to get passed context.Context parameters plus provider better diagnostics in error cases among other new features.

@andrewsomething: Could you submit this for a full run of the acceptance tests?

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Sep 23, 2020

Thanks for taking this on @tdyas! I haven't done a deep review yet, but I wanted to flag that the acceptance tests currently all fail with:

    Error: Could not load plugin
    
    
    Plugin reinitialization required. Please run "terraform init".
    
    Plugins are external binaries that Terraform uses to access and manipulate
    resources. The configuration provided requires plugins which can't be located,
    don't satisfy the version constraints, or are otherwise incompatible.
    
    Terraform automatically discovers provider requirements from your
    configuration, including providers used in child modules. To see the
    requirements and constraints, run "terraform providers".
    
    Failed to instantiate provider "registry.terraform.io/hashicorp/digitalocean"
    to obtain schema: unknown provider
    "registry.terraform.io/hashicorp/digitalocean"

Looks like resource.TestCase.Providers need to be replaced by resource.TestCase.ProviderFactories This doesn't seem well documented, but this should be a working example:

https://github.com/paultyng/terraform-provider-unifi/blob/aecb90da17c5269b7f2a859b7e1c0c204815ed23/internal/provider/provider_test.go#L13


One other thing we need to consider is that providers built with v2 of the SDK are incompatible with Terraform 0.11 cli. As a result, HashiCorp recommends the provider make a major release when doing this upgrade.

https://github.com/hashicorp/tf-sdk-migrator#tf-sdk-migrator-v2upgrade-migrate-from-sdkv1-to-sdkv2

As part of the ongoing deprecation of Terraform 0.11, version 2.0.0 of the Terraform Plugin SDK can only be used to build providers with support for Terraform 0.12 and higher.

It is recommended, but not required, that providers that have not already dropped support for 0.11 issue a major version bump when upgrading to version 2 of the SDK, to indicate this dropped support.

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Sep 24, 2020

Looks like resource.TestCase.Providers need to be replaced by resource.TestCase.ProviderFactories This doesn't seem well documented, but this should be a working example:

Pushed a change to add ProviderFactories to all of the tests.

I was able to run TestAccDigitalOceanRegions_Basic and fix an issue with it after also fixing several validation errors in the provider schemas that Terraform listed as errors at runtime during provider validation.

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Sep 24, 2020

So one thing I'm seeing is that we have a lot of tests that need to use a client directly to access the API and verify the existence of a resource. They use a pattern like:

client := testAccProvider.Meta().(*CombinedConfig).godoClient()

These are all panicking with:

panic: interface conversion: interface {} is nil, not *digitalocean.CombinedConfig

testAccProvider is not longer providing a configured client. In fact, it looks like it could be removed completely. We should be able to access the client with something lie:

provider, err := providerFactories["digitalocean"]()
if err != nil {
	return err
}
client := provider.Meta().(*CombinedConfig).godoClient()

Also, not entirely sure what happened, but there seems to be a lot of noise in the diff for the vendor directory. GitHub

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Sep 25, 2020

The AWS provider has been ported to the v2 SDK. We probably should use a similar pattern to how they consistently initialize the provider and the ProviderFactories to the same instance: https://github.com/terraform-providers/terraform-provider-aws/blob/8756a697db23a964e4af16b950af94e7c26885ab/aws/provider_test.go#L34-L59

@tdyas tdyas force-pushed the upgrade_terraform_plugin_sdk_to_v2 branch from e54089f to bd66b5c Compare Oct 2, 2020
@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 2, 2020

Rebased and squashed the commits. Found a straggler reference to the v1 SDK in the sort support in datalist package. Also fixed the provider setup in tests to set testAccProvider so that testAccProvider.Meta() works; test it by running make testacc TESTARGS='-run=^TestAccDataSourceDigitalOceanDomain_Basic'.

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Oct 2, 2020

This is fine in master, but causes a failure on all project related tests in this branch:

https://github.com/digitalocean/terraform-provider-digitalocean/blob/master/digitalocean/resource_digitalocean_project.go#L146

panic: Invalid address to set: []string{"id"}

goroutine 242 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc0004d6d80, 0x13bfff9, 0x2, 0x1194180, 0xc0005226f0, 0xc0007223f0, 0xc000486640)
	/home/asb/development/gocode/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource_data.go:188 +0x388
github.com/digitalocean/terraform-provider-digitalocean/digitalocean.resourceDigitalOceanProjectRead(0xc0004d6d80, 0x12289e0, 0xc00026ef60, 0x1, 0x1)
	/home/asb/development/gocode/src/github.com/digitalocean/terraform-provider-digitalocean/digitalocean/resource_digitalocean_project.go:147 +0x3f7
github.com/digitalocean/terraform-provider-digitalocean/digitalocean.resourceDigitalOceanProjectCreate(0xc0004d6d80, 0x12289e0, 0xc00026ef60, 0x0, 0xffffffffffffffff)
	/home/asb/development/gocode/src/github.com/digitalocean/terraform-provider-digitalocean/digitalocean/resource_digitalocean_project.go:129 +0x57d
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0xc0002520b0, 0x16de7c0, 0xc00039e200, 0xc0004d6d80, 0x12289e0, 0xc00026ef60, 0x0, 0x0, 0x0)

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Oct 2, 2020

Another panic that passes on master:

panic: droplet_id: '' expected type 'string', got unconvertible type 'int'

goroutine 23475 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc000842a80, 0x13c7831, 0xa, 0x11935c0, 0xc0003a3680, 0x0, 0x0)
	/home/asb/development/gocode/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource_data.go:188 +0x388
github.com/digitalocean/terraform-provider-digitalocean/digitalocean.resourceDigitalOceanDropletSnapshotCreate(0xc000842a80, 0x12289e0, 0xc000c9f4d0, 0x0, 0xffffffffffffffff)
	/home/asb/development/gocode/src/github.com/digitalocean/terraform-provider-digitalocean/digitalocean/resource_digitalocean_droplet_snapshot.go:80 +0x54a

Looks like this is a real bug that should be fixed either way:

https://github.com/digitalocean/terraform-provider-digitalocean/blob/master/digitalocean/resource_digitalocean_droplet_snapshot.go#L80

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 2, 2020

The panics are occurring in tests now now because the v2 SDK changes the .Set calls on ResourceData to panic now during tests instead of returning an error. https://www.terraform.io/docs/extend/guides/v2-upgrade-guide.html#louder-helper-schema-resourcedata-set-errors-in-testing

So clearly uncovering bugs as the change was made to do!

@andrewsomething
Copy link
Member

@andrewsomething andrewsomething commented Oct 2, 2020

@tdyas Definitely!

After working through those, I'm running into a number of failures to datasource tests that pass on master where the problem is not immediately clear to me. For example TestAccDataSourceDigitalOceanDomain_Basic consistently fails. Running it with debug logging, you can see the resource from the config is never even attempted to be created.

https://gist.github.com/andrewsomething/84fa93f00dbaebaf4ea1d5b974206693

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 3, 2020

After working through those, I'm running into a number of failures to datasource tests that pass on master where the problem is not immediately clear to me. For example TestAccDataSourceDigitalOceanDomain_Basic consistently fails. Running it with debug logging, you can see the resource from the config is never even attempted to be created.

I've seen this issue when I was writing tests for some prior contributions. I had to first create the resource in a separate test step (without any checks) and then do a second test step with that resource still there and the added data source:

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 3, 2020

I fixed the tests for digitalocean_certificate and digitalocean_container_registry data sources. I ran the test for digitalocean_certificate and it passes. If the fix for digitalocean_container_registry works as well, I can fix the other failing tests in a similar manner.

@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 14, 2020

hashicorp/terraform-plugin-sdk#196
Looks like the solution is about to land upstream: hashicorp/terraform-plugin-sdk#614

Probably worth vendoring those set helpers and then switching to the SDK version once that PR actually lands and is released.

@tdyas tdyas force-pushed the upgrade_terraform_plugin_sdk_to_v2 branch from f3d4cc1 to 6963b94 Compare Oct 15, 2020
@tdyas
Copy link
Contributor Author

@tdyas tdyas commented Oct 15, 2020

@andrewsomething: I brought the PR branch up to your latest commit on the other branch. I also vendored the TypeSet helpers as internal/setutil (from https://github.com/terraform-providers/terraform-provider-aws/tree/master/aws/internal/tfawsresource). This should make it easy to switch to the SDK version once the applicable PR lands, but still fix the set issues in the meantime.

@andrewsomething andrewsomething self-requested a review Oct 16, 2020
Copy link
Member

@andrewsomething andrewsomething left a comment

👍 Thanks for all your work on this @tdyas!

@andrewsomething andrewsomething merged commit f50c276 into digitalocean:master Oct 16, 2020
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants