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

Change WeatherAgent to use Openweather instead of DarkSky (which is EOL) #2848

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

erebor
Copy link
Contributor

@erebor erebor commented Jul 23, 2020

This is one option for fixing #2732 . Openweather offers a free tier that's pretty generous, and they also have done some work to making porting from DarkSky reasonably easy. It's not a perfect match, but it works pretty well.

All this PR does is remove the ForecastIO gem (which is no longer maintained and no longer needed without DarkSky) in favor of just hitting the API directly with HTTParty. There are some Openweather gems, but they're pretty rudimentary, and don't buy us much that I could see.

Changed some documentation, hopefully cleared up some errors that had crept in. Comments and suggestion welcome. I'm trying to get familiar with Huginn.

@cantino
Copy link
Member

cantino commented Jul 28, 2020

This looks really good. Maybe we wait until Aug 1 to merge?

@erebor
Copy link
Contributor Author

erebor commented Jul 28, 2020

Maybe we wait until Aug 1 to merge?

Sure! I just wanted to see whether I needed to do this differently or if I was on the right track.

(What's happening Aug 1?)

@cantino
Copy link
Member

cantino commented Jul 28, 2020

I think that's the end of the Darksky API, according to your validation error?

@erebor
Copy link
Contributor Author

erebor commented Jul 28, 2020

Oh, duh, right on. 👍

@dsander
Copy link
Collaborator

dsander commented Aug 1, 2020

I agree in general the code looks good. What is a bit unclear to me is the date when the API is shut down. The Dark Sky blog says that it will still be available until the end of 2021. I don't have an API key myself, so I can not verify that.

If their API is really still working I would love to see this change as an addition to DarkSky until they really shut down to keep the functionality for existing users.

@erebor
Copy link
Contributor Author

erebor commented Aug 2, 2020

Yeah, you're right. The Android app were to be shutdown on August 1; the API is still functioning. So not sure what you wanna do. I don't see the point in creating a separate agent for 4 months; perhaps just let the PR sit until Jan 1? Not gonna bother me. What do you guys wanna do?

@cantino
Copy link
Member

cantino commented Aug 2, 2020

I agree, we should wait to remove the Agent until the API is dead so that we don't break it for people.

@cantino
Copy link
Member

cantino commented Aug 2, 2020

Can we add OpenWeather without removing the other APIs for now?

@dsander
Copy link
Collaborator

dsander commented Aug 2, 2020

Can we add OpenWeather without removing the other APIs for now?

That would be great,

perhaps just let the PR sit until Jan 1?

That is also fine with me if you don't feel like investing more time into the change.

@bmanturner
Copy link

Dark Sky registrations are disabled. Am I supposed to wait until the end of 2021 to implement Weather events?

@cantino
Copy link
Member

cantino commented Aug 26, 2020

No, you can add it! Just don't delete support for the older APIs while they still function.

@hmnd
Copy link

hmnd commented Oct 6, 2020

I don't see the point in creating a separate agent for 4 months; perhaps just let the PR sit until Jan 1?

@erebor As far as I know, the API is being shut down at the end of 2021, not Jan. 1, 2021. I think it would be worth it to either create a separate agent or provide an option in the Weather agent to switch between the two APIs. Otherwise, it means I can't use the Weather agent at all until 2022...
If you'd prefer to not invest more time into the PR for now, would you mind if I or someone else took over where you left off and added OpenWeather as an option instead of a replacement?

@erebor
Copy link
Contributor Author

erebor commented Oct 10, 2020

If you'd prefer to not invest more time into the PR for now, would you mind if I or someone else took over where you left off and added OpenWeather as an option instead of a replacement?

@hmnd I would not mind at all! I just ran out of available time to spend on it, but I'm happy to see somebody else pick it up and run with it.

@omarabid
Copy link

@hmnd Any progress on this? It would be a good idea if the agent could support multiple API options

@tantalum
Copy link

For historical context, the weather agent originally supported two API providers, DarkSky and Weather Underground. When Weather Underground decommissioned their API I removed support for WU from the agent.

@johnmaguire
Copy link

FYI I'm working on updating this diff to support both Dark Sky and OpenWeather, at least until Dark Sky's API is decommissioned. I believe I've merged the two together, but need to sort out the tests as I'm not really a Ruby developer.

Progress here: https://github.com/JohnMaguire/huginn/tree/openweather

@johnmaguire
Copy link

Thanks for this jumping off point @erebor! Per @cantino and @dsander's suggestion, I implemented this as another service in the existing agent in #2900.

nighthawk pushed a commit to nighthawk/huginn that referenced this pull request May 19, 2022
- Updates the README clarifying support of OpenWeather, Dark Sky, and
  Wunderground.
- Merges pull request huginn#2848's support for OpenWeather in, without
  removing support for Dark Sky as the original pull request did.
  (Thanks Ryan Waldron for this!)

Choosing which weather provider to use is done via the `service` option
which was previously used in the WeatherAgent to support Wunderground
and Dark Sky simultaneously, until Wunderground ended support for their
API, at which time it was removed.

Since the WeatherAgent didn't require a `service` option for a while,
this diff takes the approach of defaulting to "forecastio" if no
`service` option is provided in order to avoid breaking configurations
prior to this change. However, the default `service` value for a new
configuration will be "openweather." Additionally, "forecastio" can be
explicitly specified, again to ensure compatibility with even older
configurations.

A note on testing - I am not a Ruby developer, so I am not quite sure
how best to add tests around the branching behavior that occurs in the
`model` method. Even worse, since I do not have a Dark Sky API key, I
cannot manually test it. I did manually test OpenWeather.

It would also be ideal to have tests which verify that the
translation of data occurs without error using fixtures from each
provider. Tests of this behavior did not already exist in
`weather_agent_spec.rb`, however OpenWeather  is tested indirectly by
`agent_spec.rb` among other specs. You can find these tests easily by
searching for the OpenWeather data fixture's file name
("openweather.json").

If someone were interested in creating tests of this nature in
`weather_agent_spec.rb`, you can also find a fixture file for Dark Sky
at `/spec/data_fixtures/weather.json` in any commit
prior to the commits from huginn#2848 (starting with
a61924f.)
if key_setup?
ForecastIO.api_key = interpolated['api_key']
onecall_endpoint = "http://api.openweathermap.org/data/2.5/onecall"

Choose a reason for hiding this comment

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

It sounds like new OpenWeatherMap account cannot use this API anymore. Do you think we can use https://api.openweathermap.org/data/2.5/weather instead? Doc: https://openweathermap.org/current

@cantino
Copy link
Member

cantino commented Dec 23, 2022

We can also just delete the DarkSky code since the API is gone now.

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.

9 participants