hclwrite: Add RenameAttribute on Body

[raymyers]

Adding the ability to change the name of an attribute already set on a block body. This can be used to automate renaming of terraform locals.

Without this, the only obvious approach is to remove the current attribute and add a new one, but this loses the order and comments which isn't desirable.

Usage

attr := block.Body().GetAttribute(fromName)
if attr != nil {
  attr.SetName(toName)
}

UPDATE

wasChanged := block.Body().RenameAttribute(fromName, toName)

[hashicorp-cla]

All committers have signed the CLA.

[apparentlymart]

Hi @raymyers! Thanks for working on this.

I agree that this seems like a valuable thing for hclwrite to be able to represent! My lone concern about it is that so far we intentionally put all of the attribute-manipulation methods up on Body rather than on individual Attribute so that we could maintain the required invariant that attribute names always be unique within a particular body. For example, SetAttribute will either update the value of an existing attribute in-place or append a new one, depending on whether the given name is already in use.

A hypothetical block.Body.RenameAttribute(fromName, toName) could in principle return a bool to indicate whether it succeeded or not and return false if the toName is already in use. That's a slightly-less-intuitive API design, but I think it would be a drop-in replacement for your current approach if you already know that toName isn't conflicted. I assume you'd already be checking that the given name isn't conflicted within Terraform's local value namespace, which requires that the names be unique across the entire module and therefore implies uniqueness within a particular locals block too.

What do you think? 🤔

[raymyers]

@apparentlymart

A hypothetical block.Body.RenameAttribute(fromName, toName) could in principle return a bool to indicate whether it succeeded or not and return false if the toName is already in use.

This makes sense, I've renamed SetName to private setName and added a RenameAttribute() on body that preserves the invariant and returns a bool as you suggested. I considered making it an error, but that seemed less consistent with the rest of the API - open to either.

Thanks for reviewing!

[raymyers]

Looks like CI has some linux-only failures in TestTokenGenerateConsistency now which didn't show up locally on my OSX and seem unrelated to the change. Is this a known issue?

[raymyers]

@apparentlymart Would you be able to approve the workflow to run the build?

[minamijoyo]

Hi @raymyers @apparentlymart, Thank you for working on this!

Is there anything I can help move this forward? I have another use case. Upgrading Terraform AWS provider v3 to v4 requires renaming some attributes. I’m currently removing an old attribute and adding a new one, which causes a loss of comments.
minamijoyo/tfedit#49

[minamijoyo]

Hi, I have another use case. Terraform v1.10 introduced DynamoDB-free S3-native state locking in the s3 backend, and the upcoming Terraform v1.11 will start deprecating the old dynamodb_table attribute and recommend using the new use_lockfile. hashicorp/terraform#36257

It's easy to rewrite a few files by hand, but I need to rewrite 300+ backend configurations across some repositories. I will not be alone in this transition because the dynamodb_table attribute has long been recommended as a best practice.

I believe the pattern "attribute A is deprecated, use B instead” is a generic use case. The current functionality only allows removing the old attribute and appending a new one to the end, which changes the order of the attributes. I don't want to change the order because meaningful order contributes to reducing cognitive load.

Please let me know if there is anything I can do to help move this issue forward.

[minamijoyo]

Yet another use case: #680

[minamijoyo]

If this patch is merged, I can provide a command line interface for this feature. See minamijoyo/hcledit#111 for details.
This will help the community migrate from dynamic_table to use_lockfile in Terraform v1.11.

[crw]

Hi @minamijoyo, I'll re-raise this in triage next week. Thanks!

[minamijoyo]

Hi @crw, I appreciate your support.

Hi @raymyers, Thank you for working on this. I need this functionality as well. As this branch looks so old, its CI check status is pending now. Can you rebase this patch on the latest main branch and see if all CI checks have been passed? Please let me know if you are too busy to do so; then, I can take it over and create a new pull request.

[raymyers]

@minamijoyo

Can you rebase this patch on the latest main branch and see if all CI checks have been passed?

Right behind ya! Rebased, thanks for reviving this and generally for your work on Terraform refactoring support.

[minamijoyo]

@raymyers Thanks for your quick response 🤗

[crw]

@hashicorp/team-ip-compliance we will need your approval to move this review forward, either before or after the core team reviews it. Thanks!

[minamijoyo]

Thanks to all who worked on this 🎉