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

Fix cert expiry config flow check and update #26638

Merged
merged 6 commits into from Sep 17, 2019

Conversation

@Cereal2nd
Copy link
Contributor

commented Sep 14, 2019

Description:

Related issue (if applicable): fixes #26619

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
Cereal2nd added 2 commits Sep 14, 2019
@project-bot project-bot bot added this to Needs review in Dev Sep 14, 2019
@Cereal2nd Cereal2nd changed the title Cert expiry bug Cert expiry bugfix #26619 Sep 14, 2019
Dev automation moved this from Needs review to Review in progress Sep 14, 2019
Copy link
Member

left a comment

Can we fix the call to get_cert in the config flow in this PR too?

homeassistant/components/cert_expiry/sensor.py Outdated Show resolved Hide resolved
@Cereal2nd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Can we fix the call to get_cert in the config flow in this PR too?

would be good, but i have no clue how to do it ...

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

Here:

def _test_connection(self, user_input=None):
"""Test connection to the server and try to get the certtificate."""
try:
get_cert(user_input[CONF_HOST], user_input.get(CONF_PORT, DEFAULT_PORT))
return True
except socket.gaierror:
self._errors[CONF_HOST] = "resolve_failed"
except socket.timeout:
self._errors[CONF_HOST] = "connection_timeout"
except OSError:
self._errors[CONF_HOST] = "certificate_fetch_failed"
return False

Do this:

async def _test_connection(self, user_input):
    """Test connection to the server and try to get the certificate."""
    try:
        await self.hass.async_add_executor_job(
            get_cert, user_input[CONF_HOST], user_input.get(CONF_PORT, DEFAULT_PORT))
        return True
    except socket.gaierror:
        self._errors[CONF_HOST] = "resolve_failed"
    except socket.timeout:
        self._errors[CONF_HOST] = "connection_timeout"
    except OSError:
        self._errors[CONF_HOST] = "certificate_fetch_failed"
    return False

And here:

if self._test_connection(user_input):

Do this:

if await self._test_connection(user_input):
@Cereal2nd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

i was trying something like that but it did not work ...

  File "/home/cereal/home-assistant/homeassistant/components/cert_expiry/config_flow.py", line 64, in async_step_user
    if await self._test_connection(user_input):
  File "/home/cereal/home-assistant/homeassistant/components/cert_expiry/config_flow.py", line 45, in _test_connection
    get_cert(user_input[CONF_HOST], user_input.get(CONF_PORT, DEFAULT_PORT))
  File "/usr/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
TypeError: 'dict' object is not callable
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

We can't call get_cert before we send it to the executor via hass.async_add_executor_job. We should just pass the function and it's arguments.

await self.hass.async_add_executor_job(get_cert, arg1, arg2)
@Cereal2nd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Thanks i really have some learning todo with the async io system ....

All items are solved now, so lets hope we can get it fixed before 0.99 is released

Copy link
Member

left a comment

Thanks!

Dev automation moved this from Review in progress to Reviewer approved Sep 15, 2019
@MartinHjelmare MartinHjelmare added this to the 0.99 milestone Sep 15, 2019
@MartinHjelmare MartinHjelmare changed the title Cert expiry bugfix #26619 Fix cert expiry config flow check and update Sep 15, 2019
@luca-angemi

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

Hi guys, not sure if this is the right place to comment but I've applied these changes to the code and the behaviour is still the same: no sensor gets created using yaml and I get a timeout error using config flow.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Can you test the site/certificate separately?

@Cereal2nd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

can you try with the same python as hass is running the next code?

import socket address = (<host>, <port>) socket.create_connection(address, timeout=10)

and see what it returns?

@luca-angemi

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

Sure

image

@pvizeli

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

Test fails because of wrong handling with asyncio

@balloob balloob merged commit 9114ed3 into home-assistant:dev Sep 17, 2019
9 checks passed
9 checks passed
CI Build #20190917.74 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 17, 2019
@Cereal2nd Cereal2nd deleted the Cereal2nd:cert_expiry_bug branch Sep 18, 2019
bramkragten added a commit that referenced this pull request Sep 18, 2019
* Fix typo in translations

* Work on bug #26619

* readd the homeassistant.start event

* Remove the callback

* Added the executor_job for _test_connection

* Update test_config_flow.py
@balloob balloob referenced this pull request Sep 18, 2019
@lock lock bot locked and limited conversation to collaborators Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
7 participants
You can’t perform that action at this time.