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 FreeDNS component #13526

Merged
merged 5 commits into from Mar 30, 2018

Conversation

Projects
None yet
5 participants
@bdurrer
Contributor

bdurrer commented Mar 28, 2018

Description:

This adds ads yet another "Dynamic DNS" provider: FreeDNS.
It's free, community-driven and supports thousands of domains, so I guess it fits the spirit of HA.

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

Example entry for configuration.yaml (if applicable):

freedns:
  access_token: YOUR_TOKEN
  update_interval:
    minutes: 15

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • Documentation added/updated in home-assistant.github.io
  • Tests have been added to verify that the new code works.
@asyncio.coroutine
def test_setup_fails_if_wrong_token(hass, aioclient_mock):
"""Test setup fails if first update fails through wrong token."""
params = { }

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

whitespace after '{'

params[ACCESS_TOKEN] = ""
aioclient_mock.get(
UPDATE_URL, params=params,
text='ERROR: Address 1.2.3.4 has not changed.')

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

continuation line unaligned for hanging indent

@asyncio.coroutine
def test_setup(hass, aioclient_mock):
"""Test setup works if update passes."""
params = { }

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

whitespace after '{'

@pytest.fixture
def setup_freedns(hass, aioclient_mock):
"""Fixture that sets up FreeDNS."""
params = { }

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

whitespace after '{'

UPDATE_INTERVAL = freedns.DEFAULT_INTERVAL
UPDATE_URL = freedns.UPDATE_URL
@pytest.fixture

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

expected 2 blank lines, found 1

if (url == None):
url = UPDATE_URL
if not (auth_token == None):

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

comparison to None should be 'if cond is None:'

params = None
if (url == None):

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

comparison to None should be 'if cond is None:'

from aiohttp.hdrs import USER_AGENT, AUTHORIZATION
import async_timeout
import voluptuous as vol
import yarl

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

'yarl' imported but unused

import logging
import aiohttp
from aiohttp.hdrs import USER_AGENT, AUTHORIZATION

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

'aiohttp.hdrs.AUTHORIZATION' imported but unused

https://home-assistant.io/components/freedns/
"""
import asyncio
import base64

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2018

'base64' imported but unused

@bdurrer bdurrer referenced this pull request Mar 28, 2018

Merged

Add FreeDNS component #5046

2 of 2 tasks complete
homeassistant/components/freedns.py Outdated
DEFAULT_TIMEOUT = 10
UPDATE_URL = 'https://freedns.afraid.org/dynamic/update.php'
HA_USER_AGENT = "{} {}".format(SERVER_SOFTWARE, EMAIL)

This comment has been minimized.

@pvizeli

pvizeli Mar 29, 2018

Member

Do we need this user agent header? We set already a default homeassistant agent

This comment has been minimized.

@fabaff

fabaff Mar 29, 2018

Member

Depends. At least NO-IP requires that there is an e-mail address attached to the request.

This comment has been minimized.

@bdurrer

bdurrer Mar 29, 2018

Contributor

we do not need it, the no_ip dns component does it and I thought it's a good idea. Removed.

homeassistant/components/freedns.py Outdated
timeout = config[DOMAIN].get(CONF_TIMEOUT)
update_interval = config[DOMAIN].get(CONF_UPDATE_INTERVAL)
session = hass.helpers.aiohttp_client.async_get_clientsession(False)

This comment has been minimized.

@pvizeli

pvizeli Mar 29, 2018

Member

Please don't disable the SSL verification. That is only allow for internal network stuff

This comment has been minimized.

@bdurrer

bdurrer Mar 29, 2018

Contributor

I only disabled it because the FreeDNS scripts do it too, but it works with enabled verification too. Fixed.

This comment has been minimized.

@pvizeli

pvizeli Mar 30, 2018

Member

that will be called: securityhole :) On a public service, SSL verification should be not disabled or you don't need use SSL.

homeassistant/components/freedns.py Outdated
DOMAIN: vol.Schema({
vol.Exclusive(CONF_URL, DOMAIN): cv.string,
vol.Exclusive(CONF_ACCESS_TOKEN, DOMAIN): cv.string,
vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,

This comment has been minimized.

@pvizeli

pvizeli Mar 29, 2018

Member

Do it really make sense to set a own timeout? 10sec should be enough for a web API.

This comment has been minimized.

@bdurrer

bdurrer Mar 29, 2018

Contributor

It's based on no_ip dns, but fair enough. Made it static.

homeassistant/components/freedns.py Outdated
def _update_freedns(hass, session, url, auth_token, timeout):
"""Update FreeDNS."""
headers = {
USER_AGENT: HA_USER_AGENT,

This comment has been minimized.

@fabaff

fabaff Mar 29, 2018

Member

USER_AGENT is a constant defined in aiohttp.hdrs.

This comment has been minimized.

@bdurrer

bdurrer Mar 29, 2018

Contributor

it's from there. Anyway, it's removed, as suggested by pvizeli.

homeassistant/components/freedns.py Outdated
import logging
import aiohttp
from aiohttp.hdrs import USER_AGENT

This comment has been minimized.

@houndci-bot

houndci-bot Mar 29, 2018

'aiohttp.hdrs.USER_AGENT' imported but unused

@bdurrer

This comment has been minimized.

Contributor

bdurrer commented Mar 29, 2018

@pvizeli oops, I think I broke the review tracking with an commit amend/push force.
Sorry about that! I changed all the remarked things.

@pvizeli

Looks good. Last things and we can merge it

if "has not changed" in body:
# IP has not changed.
_LOGGER.info("FreeDNS update skipped: IP has not changed.")
return True

This comment has been minimized.

@pvizeli

pvizeli Mar 30, 2018

Member

only return

This comment has been minimized.

@bdurrer

bdurrer Mar 30, 2018

Contributor

does that really make sense? it makes the function returning inconsistently (False and None instead of False and True), which is often considered bad practice. It also not in line with all the other dns components (google_domains, duckdns, no_ip).

This comment has been minimized.

@bdurrer

bdurrer Mar 30, 2018

Contributor

... and were actually created by you core guys a few months ago, so I really have to challenge that feedback.

This comment has been minimized.

@pvizeli

pvizeli Mar 30, 2018

Member

You are right, I don't see that you use the return above 👍

if "ERROR" not in body:
_LOGGER.debug("Updating FreeDNS was successful: %s", body)
return True

This comment has been minimized.

@pvizeli

pvizeli Mar 30, 2018

Member

only return

This comment has been minimized.

@bdurrer

bdurrer Mar 30, 2018

Contributor

see comments above

if "has not changed" in body:
# IP has not changed.
_LOGGER.info("FreeDNS update skipped: IP has not changed.")

This comment has been minimized.

@pvizeli

pvizeli Mar 30, 2018

Member

info is only allow for core. Use debug

This comment has been minimized.

@bdurrer

bdurrer Mar 30, 2018

Contributor

done. If this is a rule, should I add it to the style guidlines?

@pvizeli pvizeli merged commit 8fad97a into home-assistant:dev Mar 30, 2018

5 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.005%) to 93.942%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Apr 13, 2018

Merged

0.67.0 #13856

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.