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

Fixes an issue with OpenUV config import failing #17831

Merged
merged 4 commits into from Oct 27, 2018

Conversation

@bachya
Contributor

bachya commented Oct 26, 2018

Description:

Fixes an issue where OpenUV would fail in 0.81 when it was defined in configuration.yaml without specific latitude/longitude/elevation (i.e., relying on the default HASS latitude/longitude/elevation):

  1. Config data is imported from configuration.yaml.
  2. Default latitude/longitude/elevation doesn't populate properly.
  3. Subsequent API calls fail.

In addition to fixing the bug, this PR makes it so that the latitude/longitude/elevation aren't stored in the config entry unless they are explicitly provided; if the HASS latitude/longitude/elevation ever changes, users shouldn't have to reconfigure this flow.

Until this is pushed out, the issue can be fixed by deleting and re-creating the OpenUV config entry (note that this will have to occur each time HASS restarts). @balloob Given this, would love to have this fix in 0.81.1.

Breaking Change: It's possible that users of OpenUV may have to delete and recreate the config entry.

Related issue (if applicable): fixes #17830

Pull request in home-assistant.io with documentation (if applicable): N/A

Example entry for configuration.yaml (if applicable):

openuv:
  api_key: !secret openuv_api_key

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

@bachya bachya added this to the 0.81.1 milestone Oct 26, 2018

@bachya bachya self-assigned this Oct 26, 2018

@wafflebot wafflebot bot added the in progress label Oct 26, 2018

@bachya bachya requested a review from MartinHjelmare Oct 26, 2018

@bachya

This comment has been minimized.

Contributor

bachya commented Oct 26, 2018

@MartinHjelmare Would love your eyes on this given our conversation on the last one. Having the defaults as part of the config entry schema didn't cover a case, which caused this issue. Thoughts on my suggested route forward?

@bachya bachya removed the request for review from MartinHjelmare Oct 27, 2018

@bachya bachya changed the title from Fixes an issue with OpenUV config import failing to WIP: Fixes an issue with OpenUV config import failing Oct 27, 2018

@bachya bachya changed the title from WIP: Fixes an issue with OpenUV config import failing to Fixes an issue with OpenUV config import failing Oct 27, 2018

@bachya bachya requested a review from MartinHjelmare Oct 27, 2018

@MartinHjelmare

What was the problem?

@@ -76,13 +72,15 @@ def __init__(self):
if not api_key_validation:
return await self._show_form({CONF_API_KEY: 'invalid_api_key'})

if user_input.get(CONF_LATITUDE):
user_input[CONF_LATITUDE] = user_input[CONF_LATITUDE]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member

I don't understand this.

CONF_SCAN_INTERVAL: conf[CONF_SCAN_INTERVAL],
}

if conf.get(CONF_LATITUDE):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 27, 2018

Member
if CONF_LATITUDE in conf:
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@balloob

This comment has been minimized.

Member

balloob commented Oct 27, 2018

I've addressed your comments, waiting for tests to pass.

@bachya

This comment has been minimized.

Contributor

bachya commented Oct 27, 2018

@MartinHjelmare See #17830 for what the issue was.

@balloob Thanks for the assistance. Since tests are passing, I'm going to get this off our plate.

@bachya bachya merged commit a22aad5 into home-assistant:dev Oct 27, 2018

5 checks passed

Hound No violations found. Woof!
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 increased (+0.004%) to 93.225%
Details

@wafflebot wafflebot bot removed the in progress label Oct 27, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 27, 2018

It doesn't explain why the defaults in the form didn't work. Do you know?

@bachya

This comment has been minimized.

Contributor

bachya commented Oct 27, 2018

The defaults didn’t work when you were importing configuration from configuration.yaml. If the openuv section of configuration.yaml didn’t specify a latitude and longitude, for example, the import step in the config flow didn’t attach the schema’s default (HASS latitude and longitude). The defaults only seemed to apply when somebody configured the config flow from scratch.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 27, 2018

Thanks! I see it now.

balloob added a commit that referenced this pull request Oct 27, 2018

Fixes an issue with OpenUV config import failing (#17831)
* Fixes an issue with OpenUV config import failing

* Update

* Update __init__.py

* Update config_flow.py

@balloob balloob referenced this pull request Oct 27, 2018

Merged

0.81.1 #17895

@exxamalte

This comment has been minimized.

Contributor

exxamalte commented Oct 28, 2018

I'm confused. My openuv configuration was working prior to 0.81.0/.1 as expected.
After I upgraded to 0.81.0 I had 2 entries for OpenUV in the "Integrations" section - one with the 4 entities I had before and one saying "None, None" without entities. Entities were updating once after a restart and then I saw failing updates, Status 500 errors, etc.
After I upgraded to 0.81.1 and without changing my configuration file, I was now welcomed with "New devices discovered" which turned out to be OpenUV. A click on "Configure" starts with asking me for my API key.
Does that mean that OpenUV does not support being configured via file anymore at all?

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Oct 28, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 28, 2018

@exxamalte solved PRs are not the place for this.

@bachya bachya deleted the bachya:fix-17830 branch Oct 28, 2018

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