Extracting zoneminder to a new library #16527

rohankapoorcom · 2018-09-10T05:05:53Z

Description:

In #15324, @jantman was trying to expose the ZoneMinder run state to Home Assistant and @MartinHjelmare and @balloob asked for the device specific logic to be moved to a new pypi library before any new functionality changes be made.

I've taken a first stab at splitting out the platform with the camera.zoneminder component so far. I was hoping to get a quick review to confirm that I'm on the right track before I continued with migrating out switch.zoneminder and sensor.zoneminder. I have intentionally not set the REQUIREMENTS variable since I have not published the library yet.

I've spun up a new repo to hold the new library and have opened a PR in it for visibility: rohankapoorcom/zm-py#2

This should not require any configuration changes (nor any documentation updates) in Home Assistant.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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 or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

…w library

homeassistant/components/zoneminder.py

homeassistant/components/camera/zoneminder.py

homeassistant/components/zoneminder.py

MartinHjelmare · 2018-09-10T05:18:42Z

homeassistant/components/zoneminder.py



 def get_state(api_url):
    """Get a state from the ZoneMinder API service."""
-    return _zm_request('get', api_url)
+    global ZM
+    return ZM._zm_request('get', api_url)


It would make more sense to make a method get_state in the ZoneMinder class. The user of the library should probably not need to know what request method to use or what api url to use, only the state id to get. Or have specific methods for different state ids.

Agreed, this is temporary situation (so I didn't [completely] break the sensor/switch) until I was done refactoring.

MartinHjelmare · 2018-09-10T05:21:35Z

homeassistant/components/zoneminder.py



 def change_state(api_url, post_data):
    """Update a state using the Zoneminder API."""
-    return _zm_request('post', api_url, data=post_data)
+    global ZM
+    return ZM._zm_request('post', api_url, data=post_data)


See above. This would only need to accept the new state info and the state id.

houndci-bot · 2018-09-10T05:45:57Z

homeassistant/components/switch/zoneminder.py

-            'api/monitors/%i.json' % self._monitor_id
-        )
+        monitor = zoneminder.get_state(self.hass,
+                                       'api/monitors/%i.json' % self._monitor_id)


line too long (81 > 79 characters)

houndci-bot · 2018-09-10T05:45:57Z

homeassistant/components/sensor/zoneminder.py

-                                                        archived_filter)
-        )
+        event = zoneminder.get_state(self.hass,
+                                     'api/events/consoleEvents/{}{}.json'.format(


line too long (81 > 79 characters)

rohankapoorcom · 2018-09-14T07:06:28Z

@MartinHjelmare I believe everything should be all set here besides publishing zm-py to pypi and setting the REQUIREMENTS variable in the components that need it. If you think this makes sense, I'll go ahead and wrap that last part up.

MartinHjelmare

Looks very good. A question and some comments.

homeassistant/components/camera/zoneminder.py

MartinHjelmare · 2018-09-14T08:56:15Z

homeassistant/components/camera/zoneminder.py

+            CONF_NAME: monitor.name,
+            CONF_MJPEG_URL: monitor.mjpeg_image_url,
+            CONF_STILL_IMAGE_URL: monitor.still_image_url
+        }
        super().__init__(hass, device_info)


hass isn't used by the MjpegCamera init method. It would be nice to clean that up in a separate PR.

Sure, I'm happy to take care of that after this one

homeassistant/components/camera/zoneminder.py

homeassistant/components/sensor/zoneminder.py

homeassistant/components/switch/zoneminder.py

homeassistant/components/zoneminder.py

homeassistant/components/camera/zoneminder.py

rohankapoorcom · 2018-09-15T05:35:28Z

@MartinHjelmare All comments addressed, library published on pypi and REQUIREMENTS updated. Let me know if anything else is needed here.

Also since I plan on maintaining these components and own the new library, it might make sense to add me to CODEOWNERS for this component/platform.

MartinHjelmare · 2018-09-15T05:49:25Z

homeassistant/components/switch/zoneminder.py

        self._on_state = on_state
        self._off_state = off_state
        self._state = None

    @property
    def name(self):
        """Return the name of the switch."""
-        return "%s State" % self._monitor_name
+        return '{}\'s State'.format(self._monitor.name)


Looks like we have changed the name here by including an apostrophe. That will also change the entity_id and make this a breaking change. Is that necessary?

Wait, apostrophes should be removed by the core from the entity_id, so maybe this is ok.

Please confirm that we don't change the entity_id.

The apostrophe addition was to make the friendly name make more sense:
FrontDoor's State makes more grammatical sense that FrontDoors State (which was the predecessor).

I wanted to drop the s and just make it FrontDoor State but didn't because it would change the entity_id. I can confirm that the entity_id should not change. My switch is still called switch.frontdoors_state.

This did in fact change the entity names by adding an "s" :(

@mnoorenberghe I'm confused. The s was always in the name. The only change I made there was to add an '

Can you give an example of what your switch was named before/after this change?

That's incorrect, %s is for the format identifier for a string so %s would be replaced by "Basement" for example. There was no literal 's' before.

The previous code was like:

return "{} State".format(self._monitor_name)

D'oh you are right. I totally messed that up when changing it to the .format approach for string formatting. :(

@MartinHjelmare is this something we want to hotfix? Because it's now a breaking change. But fixing it would make another breaking change.

#17026 to fix this

MartinHjelmare · 2018-09-15T05:52:05Z

homeassistant/components/zoneminder.py

+                                   conf.get(CONF_PATH_ZMS),
+                                   conf.get(CONF_VERIFY_SSL))
+
+    return hass.data[DOMAIN].login()


Will this log an error and return False if eg credentials are bad?

Yes - See: https://github.com/rohankapoorcom/zm-py/blob/master/zoneminder/zm.py#L54

homeassistant/components/switch/zoneminder.py

MartinHjelmare

Awesome!

MartinHjelmare · 2018-09-15T06:45:31Z

Please make a new PR and add yourself to codeowners. 👍

rohankapoorcom · 2018-09-15T06:48:19Z

Thanks @MartinHjelmare. I'll follow up with one for removing hass from the MjpegCamera as well.

Migrating out the zoneminder platform (and camera.zoneminder) to a ne…

f56383b

…w library

homeassistant added cla-signed integration: zoneminder platform: camera.zoneminder small-pr PRs with less than 30 lines. labels Sep 10, 2018

ghost added the in progress label Sep 10, 2018

houndci-bot reviewed Sep 10, 2018

View reviewed changes

homeassistant/components/zoneminder.py Outdated Show resolved Hide resolved

homeassistant/components/zoneminder.py Outdated Show resolved Hide resolved

MartinHjelmare reviewed Sep 10, 2018

View reviewed changes

Clean up the global variable ZM usage

64e633d

homeassistant added the cla-signed label Sep 10, 2018

houndci-bot reviewed Sep 10, 2018

View reviewed changes

rohankapoorcom added 4 commits September 11, 2018 00:21

Modify camera.zoneminder to use the new Monitor class implementation

07db5f6

Refactor camera.zoneminder after latest refactor in zm-py

621e746

Implementing changes to switch.zoneminder to use zm-py native methods

3b37de8

Complete migrating over sensor.zoneminder to the zm-py library

73b2a69

MartinHjelmare reviewed Sep 14, 2018

View reviewed changes

Tweaking ZoneMinder components from code review

cdf7f3b

homeassistant added the cla-signed label Sep 15, 2018

rohankapoorcom changed the title ~~WIP: Extracting zoneminder to a new library~~ Extracting zoneminder to a new library Sep 15, 2018

Linting fixes for the zoneminder components

3df195b

homeassistant added the cla-signed label Sep 15, 2018

MartinHjelmare reviewed Sep 15, 2018

View reviewed changes

homeassistant/components/switch/zoneminder.py Outdated Show resolved Hide resolved

Directly assign value when turning on/off in switch.zoneminder

2e670bb

homeassistant added the cla-signed label Sep 15, 2018

MartinHjelmare approved these changes Sep 15, 2018

View reviewed changes

MartinHjelmare merged commit 1ca09ea into home-assistant:dev Sep 15, 2018

ghost removed the in progress label Sep 15, 2018

rohankapoorcom deleted the split-out-zoneminder branch September 15, 2018 06:47

This was referenced Sep 15, 2018

Add @rohankapoorcom to CODEOWNERS for the zoneminder platform #16627

Merged

Clean up MjpegCamera by removing unnused hass object in __init__ #16628

Merged

Add zoneminder run state sensor and service #15324

Closed

balloob mentioned this pull request Sep 28, 2018

0.79.0 #16940

Merged

rohankapoorcom mentioned this pull request Oct 1, 2018

Fix switch.zoneminder name #17026

Merged

2 tasks

fabaff mentioned this pull request Oct 1, 2018

Zoneminder Cameras no longer working v0.79.1 #17031

Closed

rohankapoorcom mentioned this pull request Oct 5, 2018

Fix zoneminder zms_url construction #17150

Merged

2 tasks

home-assistant locked and limited conversation to collaborators Feb 5, 2019

ghost removed the platform: camera.zoneminder label Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracting zoneminder to a new library #16527

Extracting zoneminder to a new library #16527

rohankapoorcom commented Sep 10, 2018 •

edited

Loading

MartinHjelmare Sep 10, 2018

rohankapoorcom Sep 10, 2018 •

edited

Loading

MartinHjelmare Sep 10, 2018

houndci-bot Sep 10, 2018

houndci-bot Sep 10, 2018

rohankapoorcom commented Sep 14, 2018

MartinHjelmare left a comment

MartinHjelmare Sep 14, 2018

rohankapoorcom Sep 15, 2018 •

edited

Loading

rohankapoorcom commented Sep 15, 2018 •

edited

Loading

MartinHjelmare Sep 15, 2018

MartinHjelmare Sep 15, 2018 •

edited

Loading

rohankapoorcom Sep 15, 2018 •

edited

Loading

MartinHjelmare Sep 15, 2018

mnoorenberghe Oct 1, 2018

rohankapoorcom Oct 1, 2018

mnoorenberghe Oct 1, 2018

mnoorenberghe Oct 1, 2018

rohankapoorcom Oct 1, 2018

rohankapoorcom Oct 1, 2018

MartinHjelmare Sep 15, 2018

rohankapoorcom Sep 15, 2018

MartinHjelmare Sep 15, 2018

MartinHjelmare left a comment

MartinHjelmare commented Sep 15, 2018

rohankapoorcom commented Sep 15, 2018

Extracting zoneminder to a new library #16527

Extracting zoneminder to a new library #16527

Conversation

rohankapoorcom commented Sep 10, 2018 • edited Loading

Description:

Checklist:

Choose a reason for hiding this comment

rohankapoorcom Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohankapoorcom commented Sep 14, 2018

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohankapoorcom Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

rohankapoorcom commented Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

MartinHjelmare Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

rohankapoorcom Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 15, 2018

rohankapoorcom commented Sep 15, 2018

rohankapoorcom commented Sep 10, 2018 •

edited

Loading

rohankapoorcom Sep 10, 2018 •

edited

Loading

rohankapoorcom Sep 15, 2018 •

edited

Loading

rohankapoorcom commented Sep 15, 2018 •

edited

Loading

MartinHjelmare Sep 15, 2018 •

edited

Loading

rohankapoorcom Sep 15, 2018 •

edited

Loading