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

Don't overwrite forecast points #5930

Merged
merged 3 commits into from May 31, 2019
Merged

Don't overwrite forecast points #5930

merged 3 commits into from May 31, 2019

Conversation

danielnelson
Copy link
Contributor

@danielnelson danielnelson commented May 31, 2019

This is my usual post merge CHANGELOG/README additions for the openweathermap plugin, and also some review comments from today, but I also noticed an issue with the way we generated points to be overwritten that I missed during review. We won't be able to do it this way since most outputs are not idempotent, and its risky even with InfluxDB because the output order is not guaranteed.

I also added a few new tags/fields I'm interested in: city name, country, cloudiness (I live in SF after all), visibility, sunrise, sunset. Sometime soon I'd like to add a geohash @goller.

Switched the tests over to the testutil metrics.go functions, since I'm still planning to get rid of the acc.AssertBlah functions eventually. Ended up removing the timeout test because of how long it takes and also because it is potentially racy, will have to investigate if we can readd this safely but for now I'm not sure.

cc @regel

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@danielnelson danielnelson added this to the 1.11.0 milestone May 31, 2019
@danielnelson danielnelson requested a review from glinton May 31, 2019 05:14
Copy link
Contributor

@regel regel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Daniel. No clouds here today, same in SF?

@danielnelson
Copy link
Contributor Author

I've never seen the sun here :P

AppId string `toml:"app_id"`
CityId []string `toml:"city_id"`
Fetch []string `toml:"fetch"`
BaseUrl string `toml:"base_url"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost in alphabetical order I see ;) not sure if it was intentional or not. (no change required, just pointing out)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ordered like in the config file, so "required" stuff up top. And also mostly random.

# response_timeout = "5s"

## Preferred unit system for temperature and wind speed. Can be one of
## "metric", "imperial", or "standard".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do prefer this comment over the one in the readme if you were to make them match.

@danielnelson danielnelson merged commit 0ca8ea1 into master May 31, 2019
@danielnelson danielnelson deleted the tidy-openweathermap branch May 31, 2019 23:22
hwaastad pushed a commit to hwaastad/telegraf that referenced this pull request Jun 13, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
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.

None yet

3 participants