Made it possible to parse yaml with null values

[anton-dessiatov]

Data source "yaml_map_of_strings" with "flatten" enabled used to crash if YAML with empty keys (key: null or just key:) was passed to it.

This PR makes yaml_map_of_strings ignore these empty keys.

[ashald]

Great catch!

I wonder if we should be silently omitting such fields or rather coercing them to empty strings? Given the resource name of yaml_map_of_strings, an absent value is effectively an empty string. 🙂

[ashald]

@anton-dessiatov are you interested in discussing/adjusting PR or should I just merge it and take care of the rest?

[anton-dessiatov]

Sorry for the delay. I have just created a quick test with two files:

main.tf:

data "yaml_map_of_strings" "yaml" {
  input   = "${file("yaml.yml")}"
  flatten = "/"
}

output "out1" {
  value = "${data.yaml_map_of_strings.yaml.output["empty_key"]}"
}

output "out2" {
  value = "${data.yaml_map_of_strings.yaml.output["key"]}"
}

yaml.yml:

empty_key:
  
key: "hi there"

My PR indeed fails on this example with Terraform complaining that

• output.out1: key "empty_key" does not exist in map data.yaml_map_of_strings.yaml.output in:

${data.yaml_map_of_strings.yaml.output["empty_key"]}

It's confusing because the key "empty_key" indeed exists in YAML file. So I agree that we have to represent null values somehow.

YAML makes a strict difference between null value (~ or null or !!null) and empty string, so maybe implicit coercion to empty string loses semantics.

On the other hand, we don't seem to have a way to represent null in Terraform anyway (and please correct me if I'm wrong here). There is a long relevant story about undefined values here: hashicorp/terraform#5471. The fact that Terraform null values can not be encoded makes it impossible for user to actually distinguish between null and an empty string, which invalidates my argument on losing semantics.

But we still could do our best to be strict. I believe a good way to go is to pass Terraform nil value for YAML null and let Terraform coerce it to whatever it wants. I have checked it and it seems to work fine for my example. This required to change types a little bit, though. I'll commit my changes in a few minutes.

[ashald]

Hi. Sorry for late reply.
There is number of tests for existing functionality - would you mind adding a test you mentioned? I'm not sure I fully understand how Terraform would handle nil so I hope test will help with that.

I may change my opinion once I see the behavior but I still think that we should force coerce any "void" value to an empty string. I believe that "explicit is better than implicit" and because data source is named "yaml_map_of_strings" it's better to make sure we have just strings in output. Also, as of now, Terraform's type system is pretty primitive and you cannot mix types in map values. Introducing nil adds ambiguity that I'd rather avoid.
Things likely change dramatically with Terraform v0.12 and HCL2.0 but it's not out yet. This entire plugin might need to be reconsidered once that's out.

In term of another aspect of ambiguity you mentioned, we may add an option to control behavior in the face of empty values - whether to skip them or coerce to empty strings. However, if we were to do so I'd err on the side of supplying it a default value to not break backward compatibility.

[bpoland]

Hi there, first off I want to say thanks @ashald for this provider -- it is nice to be able to use yaml files with terraform to share configurations between different tools. Hopefully once terraform 0.12 is out which should support arbitrary nested maps, it will be possible to read in a full yaml file into terraform and use it dynamically, without needing multiple invocations of yaml_list_of_strings and yaml_map_of strings.

Anyways, I have run into this crash when using a yaml file with empty values and my vote is to set them to empty strings instead of just dropping them from the returned value entirely.

Thanks again!

[anton-dessiatov]

I have added a test and this pull request now interprets YAML nulls (actually reflect.Invalids internally) as empty strings. Also squashed previous commits into one.

[ashald]

Just back from vacation and I'm 😍. @anton-dessiatov thank you for the contribution!

[ashald]

Also, added you to CONTRIBUTORS.md - hope you don't mind. 😊

[ashald]

Released as a new version https://github.com/ashald/terraform-provider-yaml/releases/tag/v2.0.1