Change hddtemp to always put temperature in value field

[jonaz]

Required for all PRs:

• CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
• Sign CLA (if not already signed)
• README.md updated (if adding a new plugin)

Also added unit tests which was missing in hddtemp plugin.
this should resolve #1904

Im not sure how to handle data migration? Perhaps a config entry to handle the new format which defaults to the old?

[phemmer]

I think it would be cleaner to bring that go-hddtemp package into the main hddtemp plugin. Having it as a package makes the code messy. And the new changes just end up wasting memory having to deal with interfaces, empty struct pointers, persisting the object, etc. Plus it's all of 61 lines long, not worth having an entirely separate package for.

But that's just my opinion. I'll leave it up to the repo maintainers to decide :-)

[jonaz]

That's not something my issue addresses. It only addresses #1905 and the code structure is as it was.

But my opinion is that single responsibility principle should apply here and keeping it as it is does enforce that and does also enhances composability.

[phemmer]

That's not something my issue addresses. It only addresses #1905 and the code structure is as it was.

But your PR is making design changes that are unrelated to the issue being addressed, solely for the sake of being able to create a unit test. If we're going to make design changes, we should do it right.

P.S. Recommended reading: https://blog.golang.org/organizing-go-code - "What to put into a package" section

[jonaz]

"right" is obviously opinionated. I remember reading that back in 2012. I'm not against moving the struct in go-hddtemp up one level. Feel free to do that in a separate PR if you like.

[mendelgusmao]

I'm not against the move as long as the license is kept. When I first wrote this piece of code, go-hddtemp was an external dependency and sparrc suggested to add a license then copy its entire code to telegraf's tree.

[mendelgusmao]

Also, I don't think a migration plan is needed. The sensors plugin had a major refactoring a few months ago that completely changed the schema and as long as I know - I mean "at least in Github" - there were no complaints. Considering that newer plugins have less users and the data value, I don't think we really need to be that protective to hddtemp measurements.

[goller]

Could this just be temperature instead of value?

[sparrc]

LGTM but please update the README with what the metrics look like that get collected by this plugin.

I realize this is not currently there, but please add it along with this PR.

[jonaz]

@goller changed to temperature
@sparrc updated readme with fields, tags and example output.

[sparrc]

thanks @jonaz, please update the changelog and I'll get this merged

[jonaz]

@sparrc is this considered feature or bugfix?

never mind. must be bugfix.