Skip to content

Make it possible to set system options on entry create#27612

Closed
scop wants to merge 1 commit into
home-assistant:devfrom
scop:config-entry-initial-options-v2
Closed

Make it possible to set system options on entry create#27612
scop wants to merge 1 commit into
home-assistant:devfrom
scop:config-entry-initial-options-v2

Conversation

@scop
Copy link
Copy Markdown
Member

@scop scop commented Oct 13, 2019

Description:

Make it possible to create config entries with an initial set of options and system options. This is especially useful on config import, and for some integrations it would be desirable to disable new entities by default as discussed in #26675.

This is an alternative, less intrusive approach to #27044.

Related issue (if applicable): #26675, #27044

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.

Comment thread homeassistant/config_entries.py Outdated
data=result["data"],
options={},
system_options={},
options=result.get("options"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not clear to me how these things would end up in result ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also don't see why this is necessary. There can just be defaults that will be applied, that don't need to be stored inside the options ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here's how they would be used: be692fb

Note that I'm actually only interested about a way to set system_options (and more specifically, disable_new_entities to True in there). If there's already a way to set that some existing way for newly created entities, please advise.

I don't actually care about options at least right now, it's fine for me to leave that part out if you don't like it. And as you mentioned, defaults should work there. So removed that part from the rebased PR now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, so now I finally get the use case.

I don't think that we should ever disable any entities by default. Entities don't just magically show up in your UI. You need to specify them in Lovelace if they do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One problem with Huawei LTE is that I want so support a bunch of functionality there, but the functionality requires a bunch of HTTP requests. Enabling everything would mean a dozen or so HTTP requests every X seconds to the device in the near future.

And for me, stuff does automatically appear in the UI. I just tried it out from a clean slate: removed ~/.homeassistant, fired hass up, went through onboarding without doing anything else, shut it down, configured the new reworked Huawei LTE branch code after hacking it to enable all found entities by default (well, not do anything to enablement actually) using yaml, started it up, and this is what I was greeted with: 6 new device tracker entities in the UI, and 20+ sensors. And I want to add more sensors and switches in the near future, which will just add more.

ui

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I should also note that the disabled by default way is what the reviewer requested in #26675, and that pretty much prompted a complete rewrite of the modernization, and a lot of work has been put into it. I think the suggestion is sound and the result is good, but disabled by default is an essential feature for it to produce good results -- both in what gets shown by default in the UI, as well as how much we hit the device API.

I'd rather not get into the business of selectively trying to enable some and disabling some new entities, because these devices and what they offer vary, things aren't always called the same between device and even firmware versions, etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we use the entity property entity_registry_enabled_default to control if entity is enabled or disabled by default?

https://developers.home-assistant.io/docs/en/entity_registry_disabled_by.html#integrations-setting-default-value-of-disabled_by-for-new-entity-registry-entries

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I suppose we can, but I also suppose if we set it to False for all Huawei LTE entities, all of them will be disabled by default but the setting in UI's config entry system options will continue to say that new entities are enabled by default, which is not what actually happens.

That flag would be more useful if we enabled some entity types and disabled some, but I'm still not quite convinced we should take that route for reasons given. I'm warming up to the idea though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is actually how Unifi does it. Once the option "disable wired clients" has been enabled, all new wired clients will be disabled when newly discovered.

I think that disabling ALL entities just seem like not something you would want. There will be entities that you would want to see right away, for example if it's connected or not. You wouldn't want that one to get caught in the "disable all new" system option.

@scop scop changed the title Make it possible to set options and system options on entry create Make it possible to set system options on entry create Oct 14, 2019
@scop
Copy link
Copy Markdown
Member Author

scop commented Oct 21, 2019

Withdrawing due to concerns, and #26675 uses a different/selective approach now.

@scop scop closed this Oct 21, 2019
@scop scop deleted the config-entry-initial-options-v2 branch October 21, 2019 15:13
@lock lock Bot locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed core invalid small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants