Skip to content

Fix cert expiry config flow check and update#26638

Merged
balloob merged 6 commits into
home-assistant:devfrom
cereal2nd:cert_expiry_bug
Sep 17, 2019
Merged

Fix cert expiry config flow check and update#26638
balloob merged 6 commits into
home-assistant:devfrom
cereal2nd:cert_expiry_bug

Conversation

@cereal2nd
Copy link
Copy Markdown
Contributor

@cereal2nd cereal2nd 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

@probot-home-assistant probot-home-assistant Bot added integration: cert_expiry small-pr PRs with less than 30 lines. labels Sep 14, 2019
@cereal2nd cereal2nd changed the title Cert expiry bug Cert expiry bugfix #26619 Sep 14, 2019
Comment thread homeassistant/components/cert_expiry/sensor.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

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

Comment thread homeassistant/components/cert_expiry/sensor.py Outdated
@cereal2nd
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

MartinHjelmare commented Sep 14, 2019

Here:
https://github.com/home-assistant/home-assistant/blob/41c9ed5d5133fe19515959b890f8c748bff0bfb8/homeassistant/components/cert_expiry/config_flow.py#L41-L52

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:
https://github.com/home-assistant/home-assistant/blob/41c9ed5d5133fe19515959b890f8c748bff0bfb8/homeassistant/components/cert_expiry/config_flow.py#L62

Do this:

if await self._test_connection(user_input):

@cereal2nd
Copy link
Copy Markdown
Contributor Author

cereal2nd 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
Copy link
Copy Markdown
Member

MartinHjelmare 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
Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Member

Can you test the site/certificate separately?

@cereal2nd
Copy link
Copy Markdown
Contributor Author

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?

@cereal2nd
Copy link
Copy Markdown
Contributor Author

@luca-angemi
Copy link
Copy Markdown
Contributor

Sure

image

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Sep 17, 2019

Test fails because of wrong handling with asyncio

@balloob balloob merged commit 9114ed3 into home-assistant:dev Sep 17, 2019
@cereal2nd cereal2nd deleted the cert_expiry_bug branch September 18, 2019 02:49
bramkragten pushed 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 mentioned 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

None yet

Development

Successfully merging this pull request may close these issues.

cert_expiry fails to load on 0.99.0b0

7 participants