Skip to content

Add Ombi integration#26755

Merged
MartinHjelmare merged 32 commits into
home-assistant:devfrom
larssont:ombi
Sep 22, 2019
Merged

Add Ombi integration#26755
MartinHjelmare merged 32 commits into
home-assistant:devfrom
larssont:ombi

Conversation

@larssont
Copy link
Copy Markdown
Contributor

@larssont larssont commented Sep 20, 2019

Description:

Integration for Ombi sensors and services.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

ombi:
  api_key: b5y5cxs3g8a6mskpmegdfdvvfc3o2wp4
  host: 192.168.1.120
  username: myusername42

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.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

@larssont larssont marked this pull request as ready for review September 20, 2019 05:57
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Code looks great!

Is there a chance that more platforms will be created for this integration, eg media_player, or custom services? In that case we should consider moving the config section to under the integration key and set up the sensor platform via discovery from the component module.

Comment thread homeassistant/components/ombi/sensor.py Outdated
Comment thread homeassistant/components/ombi/sensor.py Outdated
Comment thread homeassistant/components/ombi/sensor.py Outdated
Comment thread homeassistant/components/ombi/sensor.py Outdated
@larssont
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare
Alright, I looked over the API and did some tests and I think implementing some custom services would definitely be possible. Since Ombi's main purpose is to request movies and TV Shows and also approve requests, it would be feasible to implement a custom service that for instance searched and subsequently requested something on Ombi through the component. Although, I would have to update the pyombi package before implementing any of that here obviously.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Sep 20, 2019

I would recommend putting the config under the integration domain key instead and refactor to use helpers.discovery.load_platform to load the platform from the component module.

https://github.com/home-assistant/home-assistant/blob/54242cd65c1d69d88ec366f545313cf003e9cbbb/homeassistant/helpers/discovery.py#L110
We need to define a CONFIG_SCHEMA in the component and remove the PLATFORM_SCHEMA in the platform.

The ombi api instance should be created in the component and stored in hass.data, to later be accessible from the platform.

@tidusjar
Copy link
Copy Markdown

Nice!

@larssont
Copy link
Copy Markdown
Contributor Author

larssont commented Sep 22, 2019

I think everything should be included now. I separated the config from the sensor setup and added a couple of service calls for different types of requests. I've added in music support as well just for the sake of it. The only thing I'm a bit indecisive about is if I should implement some type of response on how each service request was handled, i.e. whether they succeeded or not. Let me know if you have any ideas for this.

Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/sensor.py
@MartinHjelmare
Copy link
Copy Markdown
Member

We can raise an error in the service handler on failure. This will be shown to the user.

Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/__init__.py Outdated
Comment thread homeassistant/components/ombi/sensor.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks great!

@MartinHjelmare
Copy link
Copy Markdown
Member

Please update the config example in the PR description.

@larssont
Copy link
Copy Markdown
Contributor Author

Great! Thanks for all the feedback, I'll make sure to update the docs.

@MartinHjelmare
Copy link
Copy Markdown
Member

Ping me when the yaml example above is updated and I'll merge here.

@larssont
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare Fixed!

@MartinHjelmare MartinHjelmare merged commit 60f0988 into home-assistant:dev Sep 22, 2019
@lock lock Bot locked and limited conversation to collaborators Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants