tmccombs / hcl2json

Convert hcl2 to json
Apache License 2.0
372 stars 53 forks source link

Single and Multiple provider definitions #18

Open jpreese opened 4 years ago

jpreese commented 4 years ago

Thanks a ton for exposing this functionality, it'll allow us to stop maintaining a local version of this library. Now onto the issue at hand!

@sarcasticadmin explained it best in this comment: https://github.com/open-policy-agent/conftest/issues/266#issuecomment-601969173

The overall issue is that when an HCL file has a single provider definition, the resulting JSON is just a single object.

"provider": {
    "aws": { "version": "=2.46.0", "alias": "one" }
}

But when an HCL file has multiple provider definitons of the same type, aliased, the JSON is turned into a set.

"provider" : {
    "aws": [
        { "version": "=2.46.0", "alias": "one" },
        { "version": "=2.46.0", "alias": "two" }
    ]
}

We would like to always return a set for the provider block for consistency.

Question being, do you agree with this approach as well and is something that hcl2json should do? If so, again would be more than happy to do a PR! Otherwise, we'll handle this specific conversion ourselves.

Context: https://github.com/open-policy-agent/conftest/issues/266

tmccombs commented 4 years ago

That seems reasonable. My big question is how does it decide if it should use a set/array or not? Should it always wrap objects in arrays, or have a list of keys that should always be treated as sets or arrays? If the latter, I think it's prefer if the set of keys was configurable.

jpreese commented 4 years ago

The thought would be that if specifically a provider block only has a single entry for that provider, we would force it to be a set by putting it into a slice during the conversion process. The case where there are multiple providers already puts it into a set so we're good there.

I haven't dug into the conversion logic a whole lot yet, but that would more or less be the idea.

tmccombs commented 4 years ago

I'd rather have something more general, because currently this tool could be used for more than just terraform, and even in the context of terraform, this same issue could arise in other situations. For example, if you have something like:

resource aws_security_group "test" {
  name = "test"
  ingress {
    from_port = 22
    to_port = 22
   protocol = "tcp"
  cidr_blocks = "10.0.0.0/24"
  }
}

you'll get

{
    "resource": {
        "aws_security_group": {
            "test": {
                "ingress": {
                    "cidr_blocks": "10.0.0.0/24",
                    "from_port": 22,
                    "protocol": "tcp",
                    "to_port": 22
                },
                "name": "test"
            }
        }
    }
}

but if you had two ingress rules, you would get an array for resource.aws_security_group.test.ingress rather than a single object.

jpreese commented 4 years ago

Agreed, that makes sense to me.