device tracker - tomato https support#11566
Conversation
|
Hi @GregoryDosh, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
Nice, was wondering why the routers didn't have https support. It would be ideal to have tests for the configuration at least. If you are inspired the unifi test looks like a good base to start from: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/device_tracker/test_unifi.py |
|
I was also rather surprised by the lack of https support. I can look into adding some tests based on unifi tests. |
| "-42,5500,1000,7043,0]];\n" | ||
| "dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA'," | ||
| "'0 days, 16:17:08'],['wemo','172.10.10.6','58:EF:68:00:00:00'," | ||
| "'0 days, 12:09:08']];", 200) |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| "-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00'," | ||
| "-42,5500,1000,7043,0]];\n" | ||
| "dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA'," | ||
| "'0 days, 16:17:08'],['wemo','172.10.10.6','58:EF:68:00:00:00'," |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA'," | ||
| "-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00'," | ||
| "-42,5500,1000,7043,0]];\n" | ||
| "dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA'," |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| elif "gimmie_good_data" in args[0].body: | ||
| return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA'," | ||
| "-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00'," | ||
| "-42,5500,1000,7043,0]];\n" |
There was a problem hiding this comment.
continuation line under-indented for visual indent
| return MockSessionResponse('This shouldn\'t (wldev = be here.;', 200) | ||
| elif "gimmie_good_data" in args[0].body: | ||
| return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA'," | ||
| "-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00'," |
There was a problem hiding this comment.
continuation line under-indented for visual indent
Rebase on master
pulling in dev changes so maybe python 3.4 won't fail CI
| CONF_HTTP_ID = 'http_id' | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
| _LOGGER = logging.getLogger("{}.{}".format(__name__, "Tomato")) |
There was a problem hiding this comment.
Okay. I was just pulling the logger on L51 to match here. I can revert the change and still pull L51 out.
|
What is the reason to use https with disable verify? In this case you can also use http |
|
@pvizeli Not sure I follow. You're saying that if using an address of If that's the case then I don't get that same behavior with my router. My router isn't serving up port 80 and if I don't specify https the requests package gives an error. Here are the permutations I ran: import itertools
import requests
import sys
import urllib3
host = "my-router"
scenario_types = {
"scheme": ["http", "https"],
"ports": [80, 443],
"ssl": [True, False],
"verify_ssl": [True, False, "router.pem"]
}
for scheme, port, ssl, verify_ssl in \
itertools.product(*scenario_types.values()):
address = "{}://{}:{}".format(scheme, host, port)
print("Schema {:<5}, Host {}, Port {:>3}, SSL {:<5}, Verify SSL {:<10}, Address {:<20} ".
format(scheme, host, port, str(ssl), str(verify_ssl), address), end="")
try:
if ssl:
response = requests.get(address,
timeout=3,
verify=verify_ssl)
else:
response = requests.get(address, timeout=3)
print(response.status_code)
except:
print(sys.exc_info()[0])Gives me the following output (note that a status code of 401 is actually good). |
| self.req = requests.Request( | ||
| 'POST', 'http://{}/update.cgi'.format(host), | ||
| 'POST', 'http{}://{}:{}/update.cgi'.format( | ||
| "s" if self.ssl else "", host, port |
There was a problem hiding this comment.
If ssl is true, port still defaults to 80. Is that what you were expecting?
There was a problem hiding this comment.
Oh, I hadn't even really thought much about that to be honest. I'll throw a small update in for that so that it'll default to 80 or 443 depending on if there is/isn't ssl enabled.
sdague
left a comment
There was a problem hiding this comment.
All seems like good adds to me. Thanks for this.
Description:
Adding https support to the tomato device tracker.
Still need to update documentation in other repo.Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4389
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass