-
Notifications
You must be signed in to change notification settings - Fork 589
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
hclwrite: Allow updating block type and labels #340
hclwrite: Allow updating block type and labels #340
Conversation
Hi @minamijoyo! Thanks for working on this, and sorry for the slow response. (I was on a vacation.) Overall this looks good to me! I have some thoughts about the new The existing strategy for inserting tokens into the middle of a construct is for the insertion area to be represented by a separate child When I think we could take a similar approach with the labels here. Currently labels are not a separate node but are rather just a I don't mean to suggest adding a new public type What do you think about that? I would prefer to use this strategy because it is consistent with the solution to similar problems in this package, but I wonder if you already considered this approach and found a problem with it that I have not noticed. Thanks again for working on this! |
@apparentlymart Thank you for your reply. To be honest, I implemented it simply referring to the existing implementation, so I didn't come up with changing the type of labels from |
@apparentlymart I would appreciate if you could review it. |
Fixes hashicorp#338 Add methods to update block type and labels to enable us to refactor HCL configurations such as renaming Terraform resources. - `*Block.SetType(typeName string)` - `*Block.SetLabels(labels []string)` Some additional notes about SetLabels: Since we cannot assume that old and new labels are equal in length, remove old labels and insert new ones before TokenOBrace. To implement this, I also added the following methods. - `*nodes.Insert(pos *node, c nodeContent) *node` - `*nodes.InsertNode(pos *node, n *node) *node` They are similar to the existing Append / AppendNode, but insert a node before a given position.
… in parser While implementing Block.SetLabels(), I found a new hclwrite parser bug. The NewBlock() method records positions of TokenOBrace / TokenCBrace. Nevertheless when generating blocks via hclwrite.ParseConfig(), they were not recorded. The position of TokenOBrace is needed for Block.SetLabels(), so I also fixed this existing bug.
All of the other subdivisions of a block were already nodes, but we'd represented the labels as an undifferentiated set of nodes belonging directly to the block's child node list. Now that we support replacing the labels in the public API, that's a good excuse to refactor this slightly to make the labels their own node. As well as being consistent with everything else in Block, this also makes it easier to implement the Block.SetLabels operation because we can just change the children of the labels node, rather than having to carefully identify and extract the individual child nodes of the block that happen to represent labels. Internally this models the labels in a similar sort of way as the content of a body, although we've kept the public API directly on the Block type here because that's a more straightforward model for the use-cases we currently know and matches better with the API of hcl.Block. This is just an internal change for consistency. I also added a few tests for having comments interspersed with labels while I was here, because that helped to better exercise the new parseBlockLabels function.
436ebf7
to
7e5b148
Compare
Hi @minamijoyo! Sorry for the very long delay in replying to you here. Unfortunately I had some unexpected high-priority projects in the meantime that meant I couldn't spend time on HCL. Your changes here look great to me, including all of the tests which made it very easy to review. The (pre-existing) inconsistency with I'm going to merge this now. I'm going to wait to make a new HCL release because we may merge some other changes before tagging, but it should be in a tagged release soon. Thanks again! |
@apparentlymart Thank you for your support and refactoring 🎉 🎉 🎉 |
Previously the block mv command depends on the forked version of the hclwrite package. A good news is the patch have been merged into upstream. So we no longer need the forked version, and simply use it with upstream. hashicorp/hcl#340
Fixes #338
Add methods to update block type and labels to enable us to refactor HCL configurations such as renaming Terraform resources.
*Block.SetType(typeName string)
*Block.SetLabels(labels []string)
Some additional notes about SetLabels:
Since we cannot assume that old and new labels are equal in length, remove old labels and insert new ones before TokenOBrace.
To implement this, I also added the following methods.
*nodes.Insert(pos *node, c nodeContent) *node
*nodes.InsertNode(pos *node, n *node) *node
They are similar to the existing Append / AppendNode, but insert a node before a given position.
While implementing Block.SetLabels(), I found a new hclwrite parser bug.
The NewBlock() method records positions of TokenOBrace / TokenCBrace.
Nevertheless when generating blocks via hclwrite.ParseConfig(), they were not recorded.
The position of TokenOBrace is needed for Block.SetLabels(), so I also fixed this existing bug.