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: Remove data source and resource id attribute requirement #164

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 21, 2023

Closes #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 identifier 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 added the enhancement New feature or request label Aug 21, 2023
@bflad

This comment was marked as outdated.

@bflad bflad force-pushed the bflad/remove-id-attribute-requirement branch from e71ac38 to de91fc1 Compare August 28, 2023 11:59
…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 force-pushed the bflad/remove-id-attribute-requirement branch from de91fc1 to 1bf0c58 Compare August 28, 2023 12:00
@bflad bflad marked this pull request as ready for review August 28, 2023 12:01
@bflad bflad requested a review from a team as a code owner August 28, 2023 12:01
@bflad bflad added this to the v1.5.0 milestone Aug 28, 2023
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🚀

@bflad bflad merged commit 18deced into main Aug 28, 2023
6 checks passed
@bflad bflad deleted the bflad/remove-id-attribute-requirement branch August 28, 2023 14:42
@github-actions
Copy link

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.

@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
Development

Successfully merging this pull request may close these issues.

helper/resource: Try to Remove id Attribute Requirement
2 participants