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

helper/resource: Try to Remove id Attribute Requirement #84

Closed
bflad opened this issue Oct 3, 2022 · 9 comments · Fixed by #164
Closed

helper/resource: Try to Remove id Attribute Requirement #84

bflad opened this issue Oct 3, 2022 · 9 comments · Fixed by #164
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bflad
Copy link
Member

bflad commented Oct 3, 2022

SDK version

v2.23.0

Use-cases

Providers written using terraform-plugin-go and terraform-plugin-framework do not require an id schema attribute, nor is it required by Terraform or the protocol. There are legacy reasons why terraform-plugin-sdk requires the id attribute but maybe that can be removed from the testing framework. This would prevent those providers from needing to declare placeholder id attributes in their schemas to prevent this type of error:

testing_new_config.go:111: no "id" found in attributes
testing_new.go:53: no "id" found in attributes

Then needing to discover the callout in the documentation.

Attempted Solutions

Placeholder id schema attributes.

Proposal

See if we cannot convince the legacy Terraform resource state handling underlying helper/resource that it is okay that we do not have an id schema attribute.

References

@bflad bflad added the enhancement New feature or request label Oct 3, 2022
@bflad
Copy link
Member Author

bflad commented Oct 3, 2022

Setting up unit testing of this will likely require some terraform-plugin-go code under ProtoV5ProviderFactories/ProtoV6ProviderFactories, since most hashicorp namespace external providers generally implement the id attribute since they were originally written with terraform-plugin-sdk.

@apparentlymart
Copy link
Member

The helper/resource test code is doing a lot of unfortunate work to try to shim the modern state model down to the old terraform.State family of structures, which did unfortunately treat ID as special at that point due to being originally shared between Terraform Core and the SDK: https://github.com/hashicorp/terraform-plugin-sdk/blob/3495894b26ae61cf11da2669170ff9d1c655896b/helper/resource/state_shim.go

In fact, that does seem to be where the error message you mentioned is coming from:

https://github.com/hashicorp/terraform-plugin-sdk/blob/3495894b26ae61cf11da2669170ff9d1c655896b/helper/resource/state_shim.go#L189-L191

I suppose it might work to just let that be missing or empty and to leave the InstanceState.ID field empty in that case, because SDKv2 can never produce a structure like that in practice (the protocol 5/6 shims will reduce it to a wholly-null result) and tests intended to be used with framework (or plugin-go) providers will presumably just ignore that ID field anyway, because it's historically always just been an alias for the attribute named id and a provider that doesn't define id has no reason to be testing against that attribute.


With that said: it seems kinda awkward to force testing for modern providers to still run through the old SDK shims, because the shims are inherently lossy: we need to reduce everything into flatmap.

Do you think it would be plausible to add a new field alongside the legacy Check which takes the state in a form that doesn't require shimming? I'm imagining that the harness that runs the tests would fail if a particular test case tries to set both the old field and the new field at the same time, and would skip running the old shims if the new field is set.

https://github.com/hashicorp/terraform-plugin-sdk/blob/3495894b26ae61cf11da2669170ff9d1c655896b/helper/resource/testing.go#L484-L485

Since the error you referred to seems to belong to the shims, I'm expecting that skipping running the shims altogether would effectively skip any legacy codepath that thinks id is special.

I suppose a key downside is that we have written various helper functions with the signature defined in resource.TestCheckFunc, and so we'd need to provide modern equivalents of some or all of those which match the signature of the new thing. 😖

@bflad
Copy link
Member Author

bflad commented Oct 11, 2022

Do you think it would be plausible to add a new field alongside the legacy Check which takes the state in a form that doesn't require shimming?

We have plans over the next few months to investigate and implement the next iteration of provider testing. That project, which wouldn't be reflected in this codebase, has a lot of considerations already so that seems like a more appropriate time for introducing larger changes rather than trying to reflect them as part of a targeted effort to try clearing up this particular attribute requirement. Unless, of course, those shims are a major blocker towards this effort and we decide that fixing this is worth potentially burdening provider developers twice with changes (once to update existing Check using this framework, then whatever code changes would be necessary for the likely new testing framework).

@bflad
Copy link
Member Author

bflad commented Feb 28, 2023

With the introduction of the terraform-plugin-testing module (migration guide), we are transferring feature requests relating to the helper/resource "testing framework" in the terraform-plugin-sdk repository over to the new terraform-plugin-testing repository.

@bflad bflad transferred this issue from hashicorp/terraform-plugin-sdk Feb 28, 2023
@ArnoSen
Copy link

ArnoSen commented Mar 22, 2023

Is there any update?
I am writing a new provider from scratch and I just ran into these errors as well:

testing_new_config.go:111: no "id" found in attributes
testing_new.go:53: no "id" found in attributes

Are there any recommendations how to circumvent the test braking?

@bflad
Copy link
Member Author

bflad commented Mar 22, 2023

@ArnoSen no updates at the moment. Please refer to the framework testing documentation for example code to implement the id attribute.

@ArnoSen
Copy link

ArnoSen commented Mar 22, 2023

Thanks for the swift response.
I am not so deep into the terraform state object yet, but will it be possible to get rid of the id once the testing framework has been updated? So in other words: what happens with a property when it is in state but not in the provider resource anymore? Or should it then be marked as Deprecated?

@bflad
Copy link
Member Author

bflad commented Mar 22, 2023

what happens with a property when it is in state but not in the provider resource anymore?

Terraform will remove the attribute and its value from state once the change is applied. Any configuration references or other usage of the non-existent attribute will raise validation errors before it can be planned/applied.

should it then be marked as Deprecated?

If you have no real world use for an id attribute, then immediately marking it as deprecated seems ideal to prevent practitioners from unexpectedly referencing it or relying on its value in configurations.

bflad added a commit that referenced this issue Aug 21, 2023
…rement

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.
bflad added a commit that referenced this issue Aug 21, 2023
…rement

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.
@bflad bflad self-assigned this Aug 22, 2023
@bflad bflad added this to the v1.5.0 milestone Aug 22, 2023
bflad added a commit that referenced this issue Aug 28, 2023
…rement

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.
bflad added a commit that referenced this issue Aug 28, 2023
…rement

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.
bflad added a commit to hashicorp/terraform-plugin-framework that referenced this issue Aug 28, 2023
…ction

Reference: hashicorp/terraform-plugin-testing#84

terraform-plugin-testing version 1.5.0 and later will no longer require managed resources and data resources to implement the `id` attribute.
bflad added a commit that referenced this issue Aug 28, 2023
…rement (#164)

Reference: #84

The resource instance state identifier was a Terraform versions 0.11 and earlier concept which helped core and the then SDK determine if the resource should be removed and as an identifier value in the human readable output. This concept unfortunately carried over to the testing logic when the testing logic was mostly changed to use the public, machine-readable JSON interface with Terraform, rather than reusing prior internal logic from Terraform. Using the `id` attribute value for this identifier was the default implementation and therefore those older versions of Terraform required the attribute. This is no longer necessary after Terraform versions 0.12 and later. This testing logic only supports Terraform version 0.12.26 and later.

The remaining resource instance identifier and `id` attribute usage in the testing logic was:

- When the `TestStep` type `ImportStateVerify` field is enabled, the instance state identifier between prior resource state and import resource state are compared to find the correct resource.
- When the `TestStep` type `ImportStateIdFunc` and `ImportStateId` fields are empty as the import ID for calling `terraform import` command.
- Other various `terraform` package references

For the first case, a new `TestStep` type `ImportStateVerifyIdentiferAttribute` field is added, which defaults to the prior `id` attribute. This preserves the existing behavior while enabling developers to choose a different identifier attribute if omitted from the resource so they can still use the `ImportStateVerify` functionality.

For the second case, developers can use the `ImportStateId*` fields to choose/create the correct import identifier value.

For the final cases, no action is needed as the `terraform` package is not intended for external usage and any usage of this identifer is mainly for equality and printing debugging information which is not accessed by the testing logic. These functions will be marked with Go documentation deprecation comments in a subsequent change.
bflad added a commit to hashicorp/terraform-plugin-framework that referenced this issue Aug 28, 2023
…ction (#830)

Reference: hashicorp/terraform-plugin-testing#84

terraform-plugin-testing version 1.5.0 and later will no longer require managed resources and data resources to implement the `id` attribute.
@github-actions
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 Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
3 participants