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

Use voluptuous for iTunes #3344

Merged
merged 2 commits into from
Sep 14, 2016
Merged

Use voluptuous for iTunes #3344

merged 2 commits into from
Sep 14, 2016

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Sep 12, 2016

Description:
Migration of the configuration check to voluptuous. This is a breaking change as http:// will no longer be needed for the host: configuration variable. Not sure if itunes-api supports SSL/TLS.

Related issue (if applicable): fixes 127528299

Example entry for configuration.yaml (if applicable):

media_player:
  platform: itunes
  name: iTunes
  host: 192.168.1.50
  port: 8181

@maddox, would be nice if you could take a look at the changes and run a quick test. Thanks.

@fabaff fabaff mentioned this pull request Sep 12, 2016
@@ -39,23 +52,23 @@ def __init__(self, host, port):
def _base_url(self):
"""Return the base url for endpoints."""
if self.port:
return self.host + ":" + str(self.port)
return 'http://{}:{}'.format(self.host, self.port)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably support an ssl: true configuration in case this is behind SSL.

I personally previously did use itunes-api behind an nginx proxy, though I had HA configured to talking to it over localhost get those extra milliseconds, heh. But regardless, people might be behind HTTPS.

@maddox
Copy link
Contributor

maddox commented Sep 12, 2016

Not sure if itunes-api supports SSL/TLS

It doesn't natively, but it could very well be behind an NGINX proxy*

*my comments on this are on the commit.

@fabaff
Copy link
Member Author

fabaff commented Sep 13, 2016

Support for SSL/TLS was added. This will need a test from a person who is running itunes-api.

response = requests.delete(url)
response = requests.get(url, timeout=DEFAULT_TIMEOUT)
elif method == 'POST':
response = requests.put(url, params, timeout=DEFAULT_TIMEOUT)
Copy link
Member

Choose a reason for hiding this comment

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

This should be requests.post ?

@balloob
Copy link
Member

balloob commented Sep 14, 2016

It's compatible with the old code, so 🐬

@balloob balloob merged commit 4791b56 into home-assistant:dev Sep 14, 2016
@maddox
Copy link
Contributor

maddox commented Sep 14, 2016

👍

@fabaff fabaff deleted the voluptuous-itunes branch September 16, 2016 08:35
hartmms pushed a commit to hartmms/home-assistant that referenced this pull request Sep 17, 2016
* Migrate to voluptuous

* Add support for SSL/TLS
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
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.

3 participants