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

Feature Request: Rename labels of block with hclwrite #338

Closed
minamijoyo opened this issue Jan 24, 2020 · 4 comments
Closed

Feature Request: Rename labels of block with hclwrite #338

minamijoyo opened this issue Jan 24, 2020 · 4 comments

Comments

@minamijoyo
Copy link
Contributor

I'm refactoring my Terraform configuration. The code has approximately 30K+ lines, so it's hard to do by hand and I looking for a way to writing automation scripts using hclwrite package in v2.
There are several operations required for refactoring, but I noticed that labels of block could not be updated. The current implementation does not have public API to set labels, so I tried to create a new block and replace it, but no luck.

The following is an example code simplified my attempts to clarify what expected and got. The result obviously has a lot of problems.

module tmp

go 1.13

require (
	github.com/davecgh/go-spew v1.1.1
	github.com/hashicorp/hcl/v2 v2.3.1-0.20200109190220-cec773f97491
)
package main

import (
	"fmt"

	"github.com/hashicorp/hcl/v2"
	"github.com/hashicorp/hcl/v2/hclwrite"
)

func main() {
	src :=
		`a0 = v0

// block comment
b1 l11 l12 {
  // body comment

  // attr comment
  a2 = v2 // inline comment
}

b2 l2 {
}
`

	want :=
		`a0 = v0

// block comment
b1 l11 l13 {
  // body comment

  // attr comment
  a2 = v2 // inline comment
}

b2 l2 {
}
`
	fromTypeName := "b1"
	fromLables := []string{"l11", "l12"}
	toTypeName := "b1"
	toLabels := []string{"l11", "l13"}

	f, _ := hclwrite.ParseConfig([]byte(src), "", hcl.Pos{Line: 1, Column: 1})
	fromBlock := f.Body().FirstMatchingBlock(fromTypeName, fromLables)

	toBlock := hclwrite.NewBlock(toTypeName, toLabels)
	toBlock.Body().AppendUnstructuredTokens(fromBlock.Body().BuildTokens(nil))
	f.Body().RemoveBlock(fromBlock)
	f.Body().AppendBlock(toBlock)

	got := string(hclwrite.Format(f.BuildTokens(nil).Bytes()))
	if got != want {
		fmt.Printf("got:\n%s\nwant:\n%s\n", got, want)
	}
}
$ go run main.go
got:
a0 = v0


b2 l2 {
}
b1 "l11" "l13" {

  // body comment

  // attr comment
  a2 = v2 // inline comment
}

want:
a0 = v0

// block comment
b1 l11 l13 {
  // body comment

  // attr comment
  a2 = v2 // inline comment
}

b2 l2 {
}

The result contains the following problems:

  1. The order of blocks can not keep
  2. The comments before block are lost
  3. An empty line is inserted before body comment
  4. The labels are implicitly treated as quoted literal.

In my cases, 3 and 4 are not serious problems.
I think replacing the existing block with a new one looks bad way.

Is it acceptable to add an API to update labels of block?
If so, what interface should be?
Once the design are agreed, I will try to implement it.

@apparentlymart
Copy link
Contributor

Hi @minamijoyo! Thanks for starting this discussion, and sharing your use-case.

After reviewing the current Block-related functions in package hclwrite, I see that so far we have the following relevant functionality:

  • NewBlock(typeName string, labels []string) *Block to construct a new block with specific type and labels
  • Block.Type() string to retrieve the current type name for an existing block.
  • Block.Labels() []string to retrieve the current labels for an existing block.

If I recall correctly, the second two of these were added by you in an earlier PR, right? I think the most natural way to extend this API is with two setter methods corresponding to the above getter methods:

  • Block.SetType(typeName string)
  • Block.SetLabels(labels []string)

So far the precedent has been that we always manipulate all of the labels together, so I think it makes sense to continue that here and require resetting all of the labels in a single call, rather than a more complicated interface for setting individual labels. A caller that wants to update one label out of a set could use Labels to retrieve the existing ones first, presumably:

labels := block.Labels()
labels[1] = "new_name"
block.SetLabels(labels)

An advantage of this approach is that we don't need to think about special rules to handle situations where the caller tries to set a new label that doesn't already exist, or have a separate method for removing a particular label while retaining the others, etc. Instead, the caller can just manipulate the labels slice using normal Go code and then set it back.

I notice in your input you've used the alternative syntax of unquoted identifiers as labels. Unfortunately, I'm going to ask that we make .SetLabels force the quoted form because that's the idiomatic way to write labels. Unquoted identifiers continued to be supported in HCL 2 only for compatibility with some historical use in HCL 1, but they have never been the documented idiom. There is already precedent for hclwrite to change the input to be idiomatic when it's asked to make modifications (e.g. it re-aligns columns, and uses idiomatic style for any new blocks), and I want to continue that principle here. It should not change the quoting on any blocks where the caller does not call block.SetLabels, though.

Does the above API proposal seem like it would meet your needs? I've not looked in the code yet to see what the implementation complexity of this might be, but since the label tokens are already separated in order to support the Block.Labels accessor I expect that writing new tokens into the Type and Labels positions should be possible with the current AST structure.

Thanks again for sharing this use-case, and for the offer to implement it!

@minamijoyo
Copy link
Contributor Author

Hi @apparentlymart, Thank you for your quick reply.

If I recall correctly, the second two of these were added by you in an earlier PR, right?

Yeah, I'm glad to hear that you remember my PR 😉

Block.SetType(typeName string)
Block.SetLabels(labels []string)

It looks natural and good to me. I'll try to implement it.

I notice in your input you've used the alternative syntax of unquoted identifiers as labels. Unfortunately, I'm going to ask that we make .SetLabels force the quoted form because that's the idiomatic way to write labels.

It's ok to force to use quoted labels. I didn't know the fact that unquoted labels are old syntax. No problems. Thanks again!

minamijoyo added a commit to minamijoyo/hcl that referenced this issue Jan 29, 2020
Fixes hashicorp#338

Add methods to update block type and labels to enable us to easy
refactor HCL configurations.

- `*Block.SetType(typeName string)`
- `*Block.SetLabels(labels []string)`

For 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.
minamijoyo added a commit to minamijoyo/hcl that referenced this issue Jan 29, 2020
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.
@minamijoyo
Copy link
Contributor Author

@apparentlymart Done. I've just submitted #340. Please check it. Thanks!

apparentlymart pushed a commit to minamijoyo/hcl that referenced this issue Aug 21, 2020
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.
apparentlymart pushed a commit that referenced this issue Aug 21, 2020
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.
@minamijoyo
Copy link
Contributor Author

Fixed in #340.

For those interested in this, you can play with my proof of concept project.

https://github.com/minamijoyo/hcledit

Given the following hcl configuration:

[hcledit@master|✔]$ cat tmp/block.hcl
resource "foo" "bar" {
  attr1 = "val1"
}

resource "foo" "baz" {
  attr1 = "val2"
}

You can rename the label:

[hcledit@master|✔]$ cat tmp/block.hcl | hcledit block mv resource.foo.bar resource.foo.qux
resource "foo" "qux" {
  attr1 = "val1"
}

resource "foo" "baz" {
  attr1 = "val2"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants