Consistent Error Handling for Setting Terraform State

[bflad]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Resources that need to write to the Terraform State use the Terraform Plugin SDK (helper/schema.ResourceData).Set() receiver method. e.g.

// Terraform Plugin SDK v1
resource ExampleThingRead(d *schema.ResourceData, meta interface{}) error {
  // ...
  if err := d.Set("attribute_name", /* ... */); err != nil {
    return fmt.Errorf("error setting attribute_name: %w", err)
  }
  // ...
}

// Terraform Plugin SDK v2
resource ExampleThingRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostic {
  // ...
  if err := d.Set("attribute_name", /* ... */); err != nil {
    return diag.FromErr(fmt.Errorf("error setting attribute_name: %w", err))
  }
  // ...
}

Technically speaking, implementing the error checking for every attribute should be done to notify operators about the potential loss of drift detection and cover missing acceptance testing data scenarios. However for pragmatic purposes such as code readability and that most acceptance testing generally covers these types well, the error checking has been considered as optional for "simple" data types such as TypeString (string), instead suggesting it for "complex" types such as TypeList/TypeMap/TypeSet. The choice of whether or not to apply the error checking can be confusing for developers and the implementations can vary.

This issue to discuss if the handling should be standardized across all resources and attributes. If so, coming up with the desired code implementation, then the tactical details of documenting it, applying it to all existing code, and enforcing it via static analysis.

Given that our desire is to move to Terraform Plugin SDK v2 compatible resources over the coming months, it is likely best to focus on that implementation. We may want to consider our own simple diagnostic wrapper to always be used for simplicity and consistency. e.g.

resource ExampleThingRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
  var diags diag.Diagnostics
  // ...
  diags = append(diags, attributeSetDiagnostic(d.Set("attribute_name", /* ... */)))
  // ...
  return diags
}

However, it would be better if the Terraform Plugin SDK hosted the implementation details here, since it should be generic across Terraform Providers. I will follow up with those maintainers. It may be better to consider a different receiver method (e.g. d.SetWithDiag()), but that team is better suited to make that determination.

References

• provider: Switch Resources To Terraform Plugin SDK V2 WithoutTimeout Signatures #15090
• https://github.com/hashicorp/terraform-provider-aws/blob/master/docs/contributing/data-handling-and-conversion.md
• Simplify and Recommend Consistent Terraform State Writing Error Handling terraform-plugin-sdk#687

[github-actions]

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

[github-actions]

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.