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

Add system options to config entries #25926

Merged
merged 15 commits into from Aug 18, 2019

Conversation

@Kane610
Copy link
Contributor

commented Aug 13, 2019

Breaking Change:

Description:

@balloob :

So my vision is that we try to solve all the things at a Home Assistant level instead of trying to solve it inside Unifi, and then solve it in all the other integrations that follow.
I much rather have a "new entities for this config entry are disabled by default" option and as a first step, we could allow propagating the unifi options to update the config entry options
Just like we have config yaml override certain options on runtime, we could set this too
I would expect a "system level options", like we used to have entity namespace and scan interval
So new "system_options" class on config entry. entry.system_options.disable_new_entities
And since Unifi imports config entry in async_setup, we can sync latest options to it.

Related issue (if applicable): fixes #25764

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

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

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@project-bot project-bot bot added this to Needs review in Dev Aug 13, 2019

@balloob

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

We should also add the part in entity platform.

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_platform.py#L336 is where we get/create an entry. Maybe we should pass the config entry instead of just the ID. The entity registry can then extract the option from the config entry.

@MartinHjelmare
Copy link
Member

left a comment

A bit off topic:

Will this feature really solve the user story? I'm thinking the user has some devices that they want to track with entities and there are devices that they don't want to track. This option won't let them select between those easily.

Should we also add another function that let's the user manually add an entity from a list of discovered devices? This is out of scope of this PR.

homeassistant/helpers/entity_registry.py Outdated Show resolved Hide resolved
@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

It will. They will have to remove those entities that they don't want visible

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

And when they want to add a new device to track they will have to repeat that process, with possibly a lot of unwanted entities.

I don't think that sounds like a good user experience.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Isn't that a second use case? The main request is to only track device x and y.

Next feature could be what you are referring to

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I think this is one user story, and my point is that the feature in this PR, although a step in the right direction, won't solve the user story. We should be aware of that.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Ok. Let's call this an MVP of the user story then :)

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@bulbasas please don't randomly approve a draft PR that is far from done

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@balloob something like this ok? Keep signalling on changes in a future PR?

@balloob balloob referenced this pull request Aug 16, 2019
Kane610 added 7 commits Aug 14, 2019

@Kane610 Kane610 force-pushed the Kane610:config_entry_system_options branch from 5a43fcd to 08e91de Aug 17, 2019

Kane610 added 3 commits Aug 17, 2019

@Kane610 Kane610 self-assigned this Aug 17, 2019

@Kane610 Kane610 removed the small-pr label Aug 17, 2019

Dev automation moved this from Needs review to Reviewer approved Aug 17, 2019

@balloob balloob merged commit a2589f5 into home-assistant:dev Aug 18, 2019

9 of 11 checks passed

CI Build #20190817.35 had test failures
Details
CI (Tests PyTest Python37) Tests PyTest Python37 failed
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
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.03%)
Details
codecov/project 94.03% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Aug 18, 2019

@Kane610 Kane610 deleted the Kane610:config_entry_system_options branch Aug 18, 2019

@flz

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

This seems to break onboarding:

2019-08-18 16:06:42 ERROR (MainThread) [aiohttp.server] Error handling request
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_protocol.py", line 418, in start
    resp = await task
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_app.py", line 458, in _handle
    resp = await handler(request)
  File "/usr/local/lib/python3.7/site-packages/aiohttp/web_middlewares.py", line 119, in impl
    return await handler(request)
  File "/workspaces/home-assistant/homeassistant/components/http/real_ip.py", line 40, in real_ip_middleware
    return await handler(request)
  File "/workspaces/home-assistant/homeassistant/components/http/ban.py", line 73, in ban_middleware
    return await handler(request)
  File "/workspaces/home-assistant/homeassistant/components/http/auth.py", line 231, in auth_middleware
    return await handler(request)
  File "/workspaces/home-assistant/homeassistant/components/http/view.py", line 128, in handle
    result = await result
  File "/workspaces/home-assistant/homeassistant/components/config/config_entries.py", line 73, in get
    and handler.async_get_options_flow
AttributeError: type object 'MetFlowHandler' has no attribute 'async_get_options_flow'

MetFlowHandler is a data_entry_flow which doesn't have a default async_get_options_flow.

@Kane610

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

Thanks for delving into the possibly relevant PR to report an issue but no, that has to do with config entry options which is a completely different thing.

Better is to create a new issue 👍

@flz

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

Thanks for the quick reply.

I agree that MetFlowHandler is a data_entry_flow but the module does register it as a config_entry (which is incorrect I assume). However, onboarding only broke after the introduction of this commit. Happy to open a separate issue but I'm guessing this should have been caught during testing?

@lock lock bot locked and limited conversation to collaborators Aug 19, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
6 participants
You can’t perform that action at this time.