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

Logi Circle public API refactor and config flow #20624

Merged

Conversation

Projects
None yet
7 participants
@evanjd
Copy link
Contributor

commented Jan 31, 2019

Description:

This is a breaking change to the Logi Circle integration which migrates from Logitech's private API to their public API.

Breaking changes:

  • Authentication with Logi Circle's API is now performed using an authorization code grant, and is managed by the Integrations page. It's no longer possible to authenticate with a username and password directly. Please remove any existing configuration for the logi_circle integration and follow the directions here to configure the logi_circle integration with at least a client_id, client_secret, api_key and redirect_uri.
  • Logi Circle camera and sensor entities are now auto-discovered once the integration is authenticated. Users should remove the logi_circle integration from camera and sensor platforms as this is no longer supported.
  • Logi Circle services have been moved from the camera domain to the logi_circle domain, as per below:
Old name New name
camera.logi_circle_livestream_record logi_circle.livestream_record
camera.logi_circle_livestream_snapshot logi_circle.livestream_snapshot
camera.logi_circle_set_config logi_circle.set_config
  • Removed support for setting the battery_saving property using the set_config service due to lack of public API support.
  • Privacy mode is now set using the recording_mode property in the set_config service.
  • Snapshots generated using the livestream_snapshot service will return a cached image provided that cache is no older than 30 seconds. This is a restriction imposed by Logi Circle's public API.

Related issue (if applicable): fixes #18307

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

Example entry for configuration.yaml (if applicable):

logi_circle:
  client_id: YOUR_CLIENT_ID
  client_secret: YOUR_CLIENT_SECRET
  api_key: YOUR_API_KEY
  redirect_uri: YOUR_REDIRECT_URI

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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 or updated 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.

@evanjd evanjd requested a review from home-assistant/core as a code owner Jan 31, 2019

@ghost ghost added the in progress label Jan 31, 2019

@evanjd evanjd force-pushed the evanjd:logi-circle-public-api-PR1 branch from 7373b85 to 02decde Feb 13, 2019

@evanjd

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Rebased to resolve upstream conflicts.

@evanjd evanjd force-pushed the evanjd:logi-circle-public-api-PR1 branch from 02decde to 55409df Feb 14, 2019

@evanjd evanjd closed this Feb 14, 2019

@ghost ghost removed the in progress label Feb 14, 2019

@evanjd evanjd reopened this Feb 14, 2019

@ghost ghost added the in progress label Feb 14, 2019

@evanjd evanjd force-pushed the evanjd:logi-circle-public-api-PR1 branch from 55409df to 64f06b2 Feb 14, 2019

@elbogi

This comment has been minimized.

Copy link

commented Feb 19, 2019

Any progress on this PR, or anything that the community could do to help this get into the next RC?


async def async_added_to_hass(self):
"""Make entity globally accessible for use in service handler."""
self.hass.data[LOGI_CIRCLE_DOMAIN]['entities']['camera'].append(self)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 11, 2019

Member

We're not allowed to store entities in a shared container. Entities are a platform concern.

Connect the entity methods to the signals of the services for the dispatch helper here instead.

This comment has been minimized.

Copy link
@evanjd

evanjd Mar 11, 2019

Author Contributor

I've added SIGNAL_LOGI_CIRCLE_RECONFIGURE, SIGNAL_LOGI_CIRCLE_RECORD and SIGNAL_LOGI_CIRCLE_SNAPSHOT for the 3 logi circle services and added listeners to the camera entity. Hopefully that's the right approach.

Addressed in commit 3aaedb86.


async def service_handler(service):
"""Dispatch service calls to target entities."""
cameras = hass.data[DATA_LOGI]['entities']['camera']

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 11, 2019

Member

We're not allowed to access entities outside their platforms. Use our dispatch helper to signal the correct entity.

This comment has been minimized.

Copy link
@evanjd

evanjd Mar 11, 2019

Author Contributor

Addressed in commit 3aaedb86.

}

# Attribution
CONF_ATTRIBUTION = "Data provided by circle.logi.com"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 11, 2019

Member

Rename this to ATTRIBUTION.

This comment has been minimized.

Copy link
@evanjd

evanjd Mar 11, 2019

Author Contributor

Resolved during rebase

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

There are merge conflicts.

@evanjd evanjd force-pushed the evanjd:logi-circle-public-api-PR1 branch from 64f06b2 to 3aaedb8 Mar 11, 2019

@MartinHjelmare
Copy link
Member

left a comment

Looks good! Just a clean up below.


async def service_handler(service):
"""Dispatch service calls to target entities."""
params = {key: value for key, value in service.data.items()}

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 11, 2019

Member
params = dict(service.data)

This comment has been minimized.

Copy link
@evanjd

evanjd Mar 12, 2019

Author Contributor

Fixed!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Nice! We require tests for the config flow module. I should have mentioned that last round, sorry.

Look at tests for hue config flow eg.

@balloob

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Any progress on the tests?

@evanjd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@balloob Made a start, but then I got swamped. Sorry for the delay + radio silence, I should have time to pick this back up my Friday evening. I'll rebase and fix the merge conflicts at the same time.

@evanjd evanjd force-pushed the evanjd:logi-circle-public-api-PR1 branch from 6544dd2 to 8453051 Apr 8, 2019

@evanjd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@MartinHjelmare Sorry for the long delay, I've added unit tests for the config flow module and rebased to resolve all upstream conflicts.

@codecov

This comment has been minimized.

Copy link

commented Apr 8, 2019

Codecov Report

Merging #20624 into dev will decrease coverage by 0.04%.
The diff coverage is 79%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #20624      +/-   ##
==========================================
- Coverage   93.83%   93.79%   -0.05%     
==========================================
  Files         448      449       +1     
  Lines       36528    36628     +100     
==========================================
+ Hits        34275    34354      +79     
- Misses       2253     2274      +21
Impacted Files Coverage Δ
...omeassistant/components/logi_circle/config_flow.py 79% <79%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 137d804...b660936. Read the comment docs.

@MartinHjelmare
Copy link
Member

left a comment

What's the coverage now for config flow? We want 100%.

Show resolved Hide resolved CODEOWNERS
@codecov

This comment has been minimized.

Copy link

commented Apr 9, 2019

Codecov Report

Merging #20624 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #20624      +/-   ##
==========================================
+ Coverage   93.83%   93.84%   +0.01%     
==========================================
  Files         448      449       +1     
  Lines       36528    36628     +100     
==========================================
+ Hits        34275    34375     +100     
  Misses       2253     2253
Impacted Files Coverage Δ
...omeassistant/components/logi_circle/config_flow.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 137d804...3520c14. Read the comment docs.

@evanjd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@MartinHjelmare Thanks for your patience. I've added more tests for logi_circle/config_flow.py and it's now at 100% coverage.

@MartinHjelmare
Copy link
Member

left a comment

Looks great!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Please highlight the breaking changes in the PR description so it's easy to copy to the release notes. Make sure it's clear to the user what the user needs to do to cope with the changes.

Then we can merge. Ping me when ready.

@evanjd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Thanks! I've adjusted some wording, specifically domain instead of platform for the services.

@MartinHjelmare MartinHjelmare merged commit a48c0f2 into home-assistant:dev Apr 9, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.83%)
Details
codecov/project 93.84% (target 90%)
Details

@ghost ghost removed the in progress label Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.