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

Simplify postgres.yaml template #380

Merged
merged 1 commit into from Nov 14, 2016

Conversation

Projects
None yet
3 participants
@miketheman
Copy link
Collaborator

miketheman commented Nov 13, 2016

Converts the postgres template to using the "simpler" rendering
solution, instead of manually unpacking positional values in YAML.

Using in-template replacement logic, we can modify the values in-place
at the time of rendering.
This does increase memory overhead slightly, as the instances object
is copied once, as it starts out as a Chef::ImmutableArray that prevents
modification.

  • Adds spec tests for specific uses brought up in #379.
  • Updates in-recpie documentation to show that one can use either
    server or host values
  • Removes a breaking change TODO

Resolves #379

Signed-off-by: Mike Fiedler miketheman@gmail.com

@miketheman

This comment has been minimized.

Copy link
Collaborator Author

miketheman commented Nov 13, 2016

@theckman Care to try this branch?

Simplify postgres.yaml template
Converts the postgres template to using the "simpler" rendering
solution, instead of manually unpacking positional values in YAML.

Using in-template replacement logic, we can modify the values in-place
at the time of rendering.
This does increase memory overhead slightly, as the `instances` object
is copied once, as it starts out as a `Chef::ImmutableArray` that prevents
modification.

- Adds spec tests for specific uses brought up in #379.
- Updates in-recipe documentation to show that one can use either
`server` or `host` values
- Removes a breaking change TODO

Resolves #379

Signed-off-by: Mike Fiedler <miketheman@gmail.com>

@miketheman miketheman force-pushed the miketheman/postgres-template branch from 69d2536 to 9dc8263 Nov 13, 2016

@theckman

This comment has been minimized.

Copy link

theckman commented Nov 13, 2016

@miketheman I'll give it a go this afternoon. If you're curious, here is the branch I have WIP that seemed to have fixed my issues (minus the breaking changes I discussed in #379): https://github.com/theckman/chef-datadog/tree/fix_postgres_yaml -- I was going to get around to cleaning it up today, but maybe now I can be lazy.

# empty by design
# Generated by Chef, local modifications will be overwritten

<%# TODO: Breaking change, remove defaults -%>

This comment has been minimized.

Copy link
@theckman

theckman Nov 13, 2016

It's a bummer the datadog_monitor LWRP doesn't provide a mechanism for passing the raw file content in to the LWRP. Over time I've tried to avoid doing logic like this in the template file, because they can get pretty intense pretty quick.

I wonder if there is any interest in supporting that within the LWRP itself.

instance['port'] = 5432 if instance['port'].nil?
end
-%>
<%= JSON.parse(({'instances' => instances}).to_json).to_yaml %>

This comment has been minimized.

Copy link
@theckman

theckman Nov 13, 2016

Nice hack for avoiding there being a second document (---) in this file. Hadn't even thought about putting that before the init_config line and was instead just building a Hash in the template file and rendering that to YAML.

@theckman
Copy link

theckman left a comment

I've tested this with Chef: 12.15.19 on Ubuntu 16.04 with Collector (v 5.9.1). 👍

@theckman theckman referenced this pull request Nov 14, 2016

Closed

Tag / Cut new release #378

@theckman

This comment has been minimized.

Copy link

theckman commented Nov 14, 2016

Actually, there may be one breaking change in here around the relations key. I ran in to this issue as I converted more things to use the updated version.

Because each item in the relations key was treated as a string before, and it was supposed to be a hash, I ended up with the following block of code in my wrapper recipe:

tables = %w(a b c)
relations = tables.map { |t| "relation_name: #{t}" }

This is the only way I could get it to render like so:

relations:
  - relation_name: a
  - relation_name: b
  - relation_name: c

With this change the file rendered like:

relations:
  - 'relation_name: a'
  - 'relation_name: b'
  - 'relation_name: c'
@miketheman

This comment has been minimized.

Copy link
Collaborator Author

miketheman commented Nov 14, 2016

@theckman is there an issue with backing out your wrapper recipe changes? I don't think the string vs hash was ever clearly distinguished and as you've mentioned, you had to be creative in working around it, so to me that points at a bug in the approach.

@theckman

This comment has been minimized.

Copy link

theckman commented Nov 14, 2016

@miketheman I'm all 👍 for introducing this change and fixing things on my side. I had some concerns that others may have done something similar, and wanted to call it out as a consideration. I've only done it in one place so far since I'm just starting to roll out Datadog, so it's easy for me.

@olivielpeau
Copy link
Member

olivielpeau left a comment

Thanks @miketheman and @theckman, changes look good! 👌

Merging, I'll release 2.7.0 - including this - tomorrow

@olivielpeau olivielpeau merged commit 3125ccc into master Nov 14, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@olivielpeau olivielpeau added this to the 2.7.0 milestone Nov 14, 2016

@miketheman miketheman deleted the miketheman/postgres-template branch Nov 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.