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

Add Ombi integration #26755

Merged
merged 32 commits into from Sep 22, 2019

Conversation

@larssont
Copy link
Contributor

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.
@project-bot project-bot bot added this to Needs review in Dev Sep 20, 2019
@larssont larssont referenced this pull request Sep 20, 2019
2 of 2 tasks complete
larssont added 2 commits Sep 20, 2019
@larssont larssont marked this pull request as ready for review Sep 20, 2019
Copy link
Member

left a comment

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.

homeassistant/components/ombi/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/sensor.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Sep 20, 2019
@larssont

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

@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

This comment has been minimized.

Copy link
Member

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.

def load_platform(hass, component, platform, discovered, hass_config):

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

This comment has been minimized.

Copy link

commented Sep 20, 2019

Nice!

larssont added 13 commits Sep 21, 2019
@larssont

This comment has been minimized.

Copy link
Contributor Author

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.

homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/sensor.py Show resolved Hide resolved
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

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

larssont added 9 commits Sep 22, 2019
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ombi/sensor.py Outdated Show resolved Hide resolved
larssont added 4 commits Sep 22, 2019
Copy link
Member

left a comment

Looks great!

Dev automation moved this from Review in progress to Reviewer approved Sep 22, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Please update the config example in the PR description.

@larssont

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

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

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

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

@larssont

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@MartinHjelmare Fixed!

@MartinHjelmare MartinHjelmare merged commit 60f0988 into home-assistant:dev Sep 22, 2019
9 checks passed
9 checks passed
CI Build #20190922.80 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
Dev automation moved this from Reviewer approved to Done 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
Dev
  
Done
4 participants
You can’t perform that action at this time.