-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
There was a problem hiding this 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?
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"` |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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". |
There was a problem hiding this comment.
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.
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: