-
Notifications
You must be signed in to change notification settings - Fork 6
Weather alert with container #26
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
Weather alert with container #26
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.
This LGTM. Approved with the caveat (that you are aware of) that that private key needs to be removed and decommissioned. Thanks for getting this in there, support for this was spotty when the API first came out.
b876d86 to
33d90d6
Compare
OK, the PR is ready. Just for clarification, the PKEY I used was already decommissioned. I assume it must be a properly generated PKEY, but there is no need for it to be active. We now simply allow(client).to receive(:token) to avoid this issue, but it will be a landmine that future developers could step on. I've also added a fallback so the specs can run without |
83007b7 to
d851315
Compare
d851315 to
a599903
Compare
|
Thanks, I'll give it a spin in the next day or two so we can get this merged and released |
820d59b to
ab06731
Compare
This sounds like the sane thing to do here. Mocking isn't really a problem. |
The specs on the mock data were not doing what we'd expect, so I use the fixture data and make clear when we're testing a real payload. It's much easier to work with JSON as a separate file. I format the JSON with
jqfor readability.It may be better to have
Alert.WeatherAlertinstead ofWeatherAlert.Summarygiven that Summary is not the best way to describe everything.In this PR we also stifle errors from a bad PKEY and allow the specs to run without a
.envfile.