postgres: template file renders invalid file in some cases

[theckman]

Hello,

I've run in to some issues with the template file used to render the check.d configuration for Postgres. Specifically, the file is unpacking the attributes (by hand) in to YAML. Unfortunately, there are a few cases where this file is doing the wrong thing.

The first issue I ran in to is that when using the relations attribute (Array), the template expects each entry to be a string[1]. This is at odds with the spec because a relation can be defined like so:

relations:
  - relation_name: table
    schemas:
      - a
      - b

Trying to use an Array of Hashes gives me:

relations:
  - {"relation_name"=>"table","schemas"=>["a","b"]}

The second issue is that the template doesn't allow the descriptors to be an empty array[2], and omitting the key is a error according to dd-agent info. If you try to provide an empty Array for descriptors, and assuming relation is false, it renders the file as such which triggers a parsing error:

descriptors:
relation: false

I'd expect it to render like this:

descriptors: []
relation: false

My suggested fix would be to change templates/default/postgres.yaml.erb to use .to_yaml to render the specific keys and avoid trying to manually unpack attributes in to YAML:

# .to_a is used to convert the attributes so that they don't marshal custom tags
<%= { "init_config" => {}, "instances" => @instances.to_a }.to_yaml %>

Unfortunately, the template that's being rendered does some things that alter attributes to provide the correct key name[3] or provide a default value[4]. This means changing this file to use .to_yaml would introduce breaking changes.

To mitigate one of these concerns, one option would be that we can patch dd-agent to default to port 5432[5] (as well as default to localhost while we're there too).

That still leaves the host => server mismatch[3] as a breaking change. To mitigate that, we could update the postgres recipe to iterate over the instances and munge the values to either set defaults or map the names to the right values. But this feels gross to me.

What do you think? In theory, what would be needed to release a breaking change and how long would it take to see a release contain the changes?

[1] github.com/DataDog/chef-datadog/templates/default/postgres.yaml.erb#L23-L26
[2] github.com/DataDog/chef-datadog/templates/default/postgres.yaml.erb#L43-L46
[3] github.com/DataDog/chef-datadog/templates/default/postgres.yaml.erb#L6
[4] github.com/DataDog/chef-datadog/templates/default/postgres.yaml.erb#L7
[5] github.com/DataDog/dd-agent/checks.d/postgres.py#L661