fix(hcl2cdk): suffix variables and locals

[DanielMSchmidt]

Input

variable "test" {
  type = string
}

locals {
  test = "${var.test} + 1"
}

output "test" {
  value = "${local.test}"
}

Output Before PR

import * as cdktf from "cdktf";
const test = new cdktf.TerraformVariable(this, "test", {});
const test = `\${${test.value}} + 1`;
new cdktf.TerraformOutput(this, "test", {
  value: test,
});

Output After PR

import * as cdktf from "cdktf";
const testVar = new cdktf.TerraformVariable(this, "test", {});
const testLocal = `\${${testVar.value}} + 1`;
new cdktf.TerraformOutput(this, "test", {
  value: testLocal,
});

[jsteinich]

What's the motivation for this change?

[DanielMSchmidt]

@jsteinich When converting https://github.com/futurice/terraform-examples/tree/master/azure/azure_linux_docker_app_service I found that locals, variables, and outputs can have the same name. Previously we just used the name directly, this resulted in multiple variables with the same name. Now they are suffixed so no more collisions can happen

[jsteinich]

Locals are certainly often better as local variables, but I wonder if they should be translated that way or instead stay as TerraformLocal?

[jsteinich]

Previously we just used the name directly, this resulted in multiple variables with the same name. Now they are suffixed so no more collisions can happen

They will still collide in construct id space.

Probably other (if not all) variables that will encounter the same issue.

[DanielMSchmidt]

@jsteinich Ah I see. We never translated locals into TerraformLocal so that's not new / the change in this PR. This one is just around variable naming. Is there a good reason to use TerraformLocal over local constants?

[DanielMSchmidt]

I'm sorry, I meant locals and variables sharing the same name. Since locals are just constants there is no construct id involved.

[jsteinich]

Is there a good reason to use TerraformLocal over local constants?

Probably depends what the generation ends up looking like. As long as a Terraform expression is just passed along, then probably not.

I'm sorry, I meant locals and variables sharing the same name. Since locals are just constants there is no construct id involved.

The variable and the output in your example overlap.

[DanielMSchmidt]

Probably depends what the generation ends up looking like. As long as a Terraform expression is just passed along, then probably not.

It is passed along, so no problem then 👍

The variable and the output in your example overlap.

Ah snap, I'll add this to the known issues or is there a nice and easy way around it?

[jsteinich]

or is there a nice and easy way around it?

You could do the same suffix that you are using for the variable name, though there's no guarantee that you won't then collide with a user named resource.

[DanielMSchmidt]

We have this problem across all resources, I think using the same formula we use to get variable names for scopes makes sense so that they are all properly "namespaced" by their respective resource type

[jsteinich]

How feasible would it be to only do the variable qualifying if conflicts are detected?

[DanielMSchmidt]

@jsteinich I think this is much better, thank you!

[jsteinich]

I didn't think just a number was valid in hcl, but I could imagine invalid names coming up, i.e. reserved keywords.

[DanielMSchmidt]

I am checking for reserved words in JS now. Rosetta will handle / check for reserved words in other languages afaik.

[jsteinich]

Technically this could still collide, but probably don't need to worry about that.

[DanielMSchmidt]

How could it collide? There can't be two identical resources with the same name in TF, can there?

[github-actions]

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 issues. If you've 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.