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

Add Uptime Robot sensor #14631

Merged
merged 8 commits into from Jun 10, 2018

Conversation

Projects
None yet
7 participants
@ludeeus
Contributor

ludeeus commented May 25, 2018

Description:

This will add a new sensor for every monitor on that are accessable with the supplied api_key: from https://uptimerobot.com

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5437

Example entry for configuration.yaml (if applicable):

binary_sensor:
  - platform: uptimerobot
    api_key: u432898-d2507e493b31217e6c64fd35

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
    There was an issue running tox but I think that is not relevant to this:
1 failed
  - tests/components/test_feedreader.py:112 TestFeedreaderComponent.test_feed

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

I have a demo for it here.

return {
ATTR_TARGET: self._target,
ATTR_MONITORID: self._monitorid,
}

This comment has been minimized.

@houndci-bot

houndci-bot May 25, 2018

no newline at end of file

@ludeeus ludeeus referenced this pull request May 26, 2018

Merged

Documentation for Uptime Robot sensor #5437

2 of 2 tasks complete
@dgomes

This comment has been minimized.

Member

dgomes commented May 28, 2018

since you only report "online" vs "offline" why isn't this a binary_sensor ?

@ludeeus

This comment has been minimized.

Contributor

ludeeus commented May 29, 2018

@dgomes closest to this that I found for a device_class was "connectivity", but connected/disconnected didn't "feel" right :)
I can convert it if this breaks a policy.

@MartinHjelmare

I agree, a binary sensor platform is more appropriate. Looks good otherwise. Small comment about the log message style.

devices = []
if not monitors or monitors.get('stat') != 'ok':
error = monitors.get('error', {})
_LOGGER.error(error.get('message', 'Something terrible happend :('))

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 29, 2018

Member

Please keep the error log message informative and kind of professional. The latter message here is too broad and breaks the log style of home assistant. Smilies are also out of style.

Sorry for sounding boring.

This comment has been minimized.

@ludeeus

ludeeus May 29, 2018

Contributor

I'll convert it to an binary_sensor and give a proper log entry :)

@@ -33,24 +31,23 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
devices = []
if not monitors or monitors.get('stat') != 'ok':
error = monitors.get('error', {})
_LOGGER.error(error.get('message', 'Something terrible happend :('))
_LOGGER.error('Error connecting to uptime robot.')

This comment has been minimized.

@cdce8p

cdce8p May 29, 2018

Member

Maybe keep the error.get, since that is more helpful if we have an error message.

_LOGGER.error(error.get('message', 'Error connecting to uptime robot.'))

This comment has been minimized.

@ludeeus

ludeeus May 29, 2018

Contributor

The external lib, returns False if there is a timeout when connecting, that function does not handle that well :)

This comment has been minimized.

@cdce8p

cdce8p May 29, 2018

Member

That should be caught by error = monitors.get('error', {}) above.

This comment has been minimized.

@ludeeus

ludeeus May 29, 2018

Contributor

They where not, thats why I removed it :)

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented May 29, 2018

The platform is still under sensor component.

I think you need to rebase to get a clean diff in the requirements file.

@ludeeus

This comment has been minimized.

Contributor

ludeeus commented May 29, 2018

@MartinHjelmare I just forgot to move it... thanks for pointing that out :)

@dgomes dgomes added the almost-done label Jun 7, 2018

@wafflebot wafflebot bot added in progress and removed almost-done labels Jun 9, 2018

@MartinHjelmare

Some small changes needed before merge. Please also update the config section in the PR description.

https://www.home-assistant.io/components/binary_sensor.uptimerobot
"""
import logging
import voluptuous as vol

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 9, 2018

Member

Please separate the three import groups, standard library, 3rd party and homeassistant, with a blank line.

devices = []
if not monitors or monitors.get('stat') != 'ok':
_LOGGER.error('Error connecting to uptime robot.')
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 9, 2018

Member

Just return. Nothing is checking this return value.

name=monitor['friendly_name'], target=monitor['url']))
add_devices(devices, True)
return True

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 9, 2018

Member

Remove this.

monitor = self._up_robot.getMonitors(self._apikey, self._monitorid)
if not monitor or monitor.get('stat') != 'ok':
_LOGGER.debug("Failed to get new state, trying again later.")
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jun 9, 2018

Member

Just return. Nothing is checking this return value.

@MartinHjelmare

Great!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Jun 9, 2018

Can be merged when build passes.

@MartinHjelmare MartinHjelmare changed the title from Added Uptime Robot sensor to Add Uptime Robot sensor Jun 9, 2018

@MartinHjelmare MartinHjelmare merged commit ce7e9e3 into home-assistant:dev Jun 10, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 94.01%
Details

@wafflebot wafflebot bot removed the in progress label Jun 10, 2018

@ludeeus ludeeus deleted the ludeeus:uptimerobot branch Jun 10, 2018

@balloob balloob referenced this pull request Jun 22, 2018

Merged

0.72 #15088

MizterB added a commit to MizterB/home-assistant that referenced this pull request Aug 11, 2018

Add Uptime Robot sensor (home-assistant#14631)
* Added Uptime Robot sensor

* added newline at the end and corrected doclink

* Added changes form @cdce8p

* Convert to binary_sensor

* updated requirements

* moved to correct dir

* Update uptimerobot.py

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Add Uptime Robot sensor (home-assistant#14631)
* Added Uptime Robot sensor

* added newline at the end and corrected doclink

* Added changes form @cdce8p

* Convert to binary_sensor

* updated requirements

* moved to correct dir

* Update uptimerobot.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment