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

switching from toml-rb to toml gem #43

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

codylewandowski
Copy link
Contributor

This is a potential replacement to #39 and a fix to #41. I tested this in combination with the latest version of https://github.com/bdangit/chef-influxdb and everything worked as expected.

@codylewandowski
Copy link
Contributor Author

@fishnix @phromo Does this better align with the issues that you commend in #39?

@fishnix
Copy link
Contributor

fishnix commented Jun 14, 2017

thanks @codylewandowski, reviewing this now

@fishnix
Copy link
Contributor

fishnix commented Jun 14, 2017

Looks good, thanks again! 👍

@fishnix fishnix merged commit 6378daf into NorthPage:master Jun 14, 2017
@codylewandowski codylewandowski deleted the switch_toml_gem branch June 14, 2017 21:29
@codylewandowski
Copy link
Contributor Author

Thanks for the quick release too @fishnix! You rock!

@fishnix fishnix mentioned this pull request Jun 14, 2017
@joshmyers
Copy link

joshmyers commented Jun 20, 2017

Hi @fishnix @codylewandowski this commit seems to break existing behaviour when generating TOML from a ruby hash:

Given a ruby hash example:

foo = {
  "cpu" => {
    'percpu'    => true,
    'totalcpu'  => true,
    'fielddrop' => [ 'time_*' ]
  },
  "disk" =>  {
    'ignore_fs' => [ "tmpfs", "devtmpfs" ],
    'tagexclude' => ['fstype', 'path', 'device']
  },
  "diskio" => {},
  "mem"    => {},
  "net"    => {},
  "swap"   => {},
  "system" => {}
}

Expected output (when using toml-rb):

puts TomlRB.dump(foo)
[cpu]
fielddrop = ["time_*"]
percpu = true
totalcpu = true
[disk]
ignore_fs = ["tmpfs", "devtmpfs"]
tagexclude = ["fstype", "path", "device"]
[diskio]
[mem]
[net]
[swap]
[system]

Actual output (now using toml):

puts TOML::Generator.new(foo).body

[cpu]
fielddrop = ["time_*"]
percpu = true
totalcpu = true

[disk]
ignore_fs = ["tmpfs","devtmpfs"]
tagexclude = ["fstype","path","device"]

As can be seen above empty hash keys result in no Telegraf config.

Do you want me to open an issue against this?

joshmyers added a commit to alphagov/telegraf-cookbook that referenced this pull request Jun 20, 2017
This reverts commit 6c1e482.

See comment on upstream PR for this commit:
NorthPage#43
joshmyers added a commit to alphagov/telegraf-cookbook that referenced this pull request Jun 20, 2017
See comment on NorthPage#43

TOML class was renamed to TomlRB in a recent release, update.
joshmyers added a commit to alphagov/telegraf-cookbook that referenced this pull request Jun 20, 2017
See comment on NorthPage#43

TOML class was renamed to TomlRB in a recent release, update.
@phromo
Copy link
Contributor

phromo commented Jun 20, 2017

sorry for not replying to earlier PR. I fixed the issue @joshmyers refers to in phromo@17bb8b6

I've just become a father to a lovely daughter and don't have much time for computers atm :)

@jrg72
Copy link

jrg72 commented Jun 27, 2017

Thank you @phromo, your fix in that fork of yours was the solution to a non-work-hours upgrade failure for our telegraf wrapper cookbook in prod!

Though, it looks like this is still broken in master here, given that the defaults in the attributes file still defaults to => {}.

@codylewandowski
Copy link
Contributor Author

@joshmyers @phromo @jrg72 I just opened #46 to address these issues. Sorry for the delay in response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants