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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exclude option to default_config #57871

Closed
wants to merge 3 commits into from

Conversation

LouisMT
Copy link
Contributor

@LouisMT LouisMT commented Oct 16, 2021

Proposed change

The default_config integration enables a set of default integrations. Each integration in this set can still be configured by the user by simply adding it after default_config. For example:

default_config:
automation:
    # override config here

So while it's still possible to change configuration for each default integration, it's currently not possible to disable any of the default integrations.

This could be useful, for example I'm having an issue with the dhcp integration in Docker. I don't need it. But to disable it now, I must remove the default_config integration and keep track of all the default integrations myself. I've done this in the past, and it resulted in me missing out on a lot of new features in new Home Assistant releases. 馃槃

I'm creating this PR to get some feedback. I'll add docs & tests if you like my suggestion!

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@tdejneka
Copy link

This is a long-requested enhancement. Lots of feedback (and votes) here:
https://community.home-assistant.io/t/why-the-heck-is-default-config-so-difficult-to-customize/220112

@Romkabouter
Copy link
Contributor

Very usefull indeed!

@frenck frenck marked this pull request as draft October 22, 2021 19:29
@MartinHjelmare MartinHjelmare moved this from Needs review to Incoming in Dev Nov 10, 2021
@Petro31
Copy link
Contributor

Petro31 commented Dec 9, 2021

What's delaying this? Anything that can be done?

@LouisMT
Copy link
Contributor Author

LouisMT commented Dec 9, 2021

Unfortunately I got stuck, I'll briefly explain why below. Because of that and due to a lack of time I'm afraid I'll have to close this PR for now. Also, this is not required anymore for the DHCP integration, as the issue with spamming has been fixed in the integration itself.

The issue is as follows: this is fully working as is. But there's a catch: the original code cleverly used the dependencies property in the manifest, which lets the Home Assistant core itself set up each integration. My change moves this responsibility from the core into the default_config integration. This doesn't seem to be a problem normally, but the core monitors each integration for problems during setup, and each integration gets N seconds to finish setup before it gets killed. If one of the default integrations causes problems during setup now, it may prevent all other default integrations from loading. So I have a few choices:

  • Current solution: setup all integrations simultaneously, and report if all integrations have been set up successfully or not. But this would introduce a delay if a single integration takes too long. And the core can't monitor individual integrations this way.
  • Setup all integrations as fire-and-forget. If one integration fails setting up: no problem. But Home Assistant isn't monitoring anything at all this way.
  • Create a new manifest property/move this code to the core. I personally think this is the best solution: the core can set up each integration individually and monitor it, exactly like how it's being done right now. But I guess there's a reason this logic is in a separate integration: doing this will add extra complexity to the core. I'm not sure where to discuss changes like this.

I haven't looked into the core very long, so I could be wrong. If so, please correct me! 馃槃 Hopefully I can find some time to work on this soon. But if someone else wants to take over: be my guest!

@LouisMT LouisMT closed this Dec 9, 2021
Dev automation moved this from Incoming to Cancelled Dec 9, 2021
@bwosborne2
Copy link

Unfortunately I got stuck, I'll briefly explain why below. Because of that and due to a lack of time I'm afraid I'll have to close this PR for now. Also, this is not required anymore for the DHCP integration, as the issue with spamming has been fixed in the integration itself.

The issue is as follows: this is fully working as is. But there's a catch: the original code cleverly used the dependencies property in the manifest, which lets the Home Assistant core itself set up each integration. My change moves this responsibility from the core into the default_config integration. This doesn't seem to be a problem normally, but the core monitors each integration for problems during setup, and each integration gets N seconds to finish setup before it gets killed. If one of the default integrations causes problems during setup now, it may prevent all other default integrations from loading. So I have a few choices:

  • Current solution: setup all integrations simultaneously, and report if all integrations have been set up successfully or not. But this would introduce a delay if a single integration takes too long. And the core can't monitor individual integrations this way.
  • Setup all integrations as fire-and-forget. If one integration fails setting up: no problem. But Home Assistant isn't monitoring anything at all this way.
  • Create a new manifest property/move this code to the core. I personally think this is the best solution: the core can set up each integration individually and monitor it, exactly like how it's being done right now. But I guess there's a reason this logic is in a separate integration: doing this will add extra complexity to the core. I'm not sure where to discuss changes like this.

I haven't looked into the core very long, so I could be wrong. If so, please correct me! 馃槃 Hopefully I can find some time to work on this soon. But if someone else wants to take over: be my guest!

Why not just make the dependencies in /homeassistant/components/default_config/manifest.json selectable? I believe that is where the default config entry is defined.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

DHCP discovery spamming DNS PTR requests in Docker
9 participants