Deprecate knx config_file by farmio · Pull Request #46874 · home-assistant/core

farmio · 2021-02-21T22:07:52Z

Breaking change

xknx.yaml config_file is deprecated.

With this PR only a deprecation message and a warning in the logs is displayed. The config_file will continue to work as before until it is removed.

Proposed change

The KNX integration currently supports 2 configuration Schemes

HA-Schema: used in Home Assistant configuration.yaml and documented at https://www.home-assistant.io/integrations/knx/
XKNX-Schema: provided in a separate .yaml file whose path is configured with the config_file: key

Having 2 different configuration Schemes is a constant source for issues and bugs.

We want to deprecate the XKNX-Schema and only support HA-Schema in the future.
To help users with the schema transition we set up an online config-converter at https://xknx.io/config-converter/ where users can paste their XKNX-Schema and get a HA-Schema to copy to configuration.yaml .

This will allow us to take complexity out of the HA-integration and xknx at the same time. Maintainability and building new features will also be easier.

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: remove KNX config_file home-assistant.io#16684

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:

Documentation added/updated for www.home-assistant.io

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.
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:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2021-02-21T22:08:01Z

Hey there @Julius2342, @marvin-w, mind taking a look at this pull request as its been labeled with an integration (knx) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

bdraco · 2021-02-22T03:43:28Z

    {
        DOMAIN: vol.All(
+            # deprecated since 2021.3
+            cv.deprecated(CONF_KNX_CONFIG, replacement_key=CONF_KNX_CONFIG),


The 'CONF_KNX_CONFIG' option is deprecated, please replace it with 'CONF_KNX_CONFIG'

This doesn't look right that the replacement_key is the same

This doesn't really break things, it will fire a warning from the cv.deprecated config validator for now.
If I don't specify a replacement_key the config validator removes the key from config and it can not be used further down. This would result in users loosing some configuration like HomeKit room assignment (afaik).

So for now I'd like to just point out the deprecation of this config and log a link to the config-converter. After a transition period is over (maybe in summer) well remove the replacement_key and the logic using the XKNX-Schema.

I was more worried about the link to an external resource (config-converter tool) being accepted.

would it be ok to also create a persistent_notification ?

I'm not sure about an external link in a notification. I'll ask for a second opinion on that.

If it is any help: xknx.io is also the only source for documentation of the deprecated XKNX-Schema - so users of this are probably familiar with this domain.

Deprecated should not remove the key if you don't specify replacement. You have to explicitly remove it with a vol.Remove() if you want it removed.

Also we don't generally do notifications on deprecated. If we want to do that i think we should do that globally.

I've removed the deprecation config validator. It's a log normal log message and notification now. (Only when config_file is used).
Shall I remove the notification again? I think a log message can be quite invisible for users - and this change will probably render their whole installation useless (knx is usually a quite big installation). When it is deprecated there is no way to point to the converter website...

We should remove the external domain in the notification. If this is useful, add it to the documentation that is already linked instead.

TBH I'm not really sure users will be pleased and/or successful in finding that link in this 1,3k lines markdown document.

bdraco

Please see comment above

bdraco · 2021-02-22T07:32:02Z

second opinion request is for #46874 (comment)

elupus · 2021-02-22T08:35:25Z

In general +1 from me.

Co-authored-by: Philip Allgaier <philip.allgaier@gmx.de>

frenck · 2021-02-23T23:30:51Z

+    # deprecation warning since 2021.3
+    if CONF_KNX_CONFIG in config[DOMAIN]:
+        _LOGGER.warning(
+            "The 'config_file' option will soon be deprecated. Please replace it with Home Assistant config schema "


It is deprecated (see code above).

Suggested change

"The 'config_file' option will soon be deprecated. Please replace it with Home Assistant config schema "

"The 'config_file' option is deprecated. Please replace it with Home Assistant config schema "

Please note, that having both a cv.deprecated and a custom message, will now cause the user to have duplicate warnings (with different messages).

Is that intentional?

I can remove cv.deprecated again if you like.

Would it be welcome if I changed cv.deprecated to accept custom messages in general?

I can remove cv.deprecated again if you like.

I think it's better to choose one or the other. I don't think we should mailbomb a user with duplicate messages from a user experience perspective. However, I don't know if it was intentional or not, hence my question. So 🤷

Would it be welcome if I changed cv.deprecated to accept custom messages in general?

We didn't have had much need for it in general. As most are just removed after it has been automatically migrated or imported into the UI. KNX is a special case, so let's not add that for now.

Well, this could be the first step to migrate knx to config_flow...
Thanks for review!

Co-authored-by: Franck Nijhof <frenck@frenck.nl>

farmio · 2021-03-01T10:58:36Z

Hi! Will this be included in 2021.3 or 2021.4?

frenck · 2021-03-01T11:00:06Z

This PR should not have been merged, that was an unresolved comment open:

We should remove the external domain in the notification. If this is useful, add it to the documentation that is already linked instead.

#46874 (comment)

@farmio Please address that comment in a separate PR, so we can avoid reverting this one. Thanks 👍

farmio · 2021-03-01T11:40:58Z

¯_(ツ)_/¯
I'll try to do a PR tonight reverting it.

farmio · 2021-03-01T19:02:30Z

#47238

deprecate config_file

670e5a2

homeassistant added the cla-signed label Feb 21, 2021

probot-home-assistant Bot added breaking-change integration: knx small-pr PRs with less than 30 lines. by-code-owner labels Feb 21, 2021

farmio mentioned this pull request Feb 21, 2021

remove KNX config_file home-assistant/home-assistant.io#16684

Merged

8 tasks

bdraco reviewed Feb 22, 2021

View reviewed changes

bdraco requested changes Feb 22, 2021

View reviewed changes

bdraco reviewed Feb 22, 2021

View reviewed changes

Comment thread homeassistant/components/knx/__init__.py Outdated

bdraco reviewed Feb 22, 2021

View reviewed changes

Comment thread homeassistant/components/knx/__init__.py Outdated

bdraco reviewed Feb 22, 2021

View reviewed changes

Comment thread homeassistant/components/knx/__init__.py Outdated

farmio added 2 commits February 22, 2021 07:47

Merge remote-tracking branch 'upstream/dev' into deprecate-config_file

09edc57

removed cv.deprecated for now, added persistent notification

77befe5

bdraco added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 22, 2021

MartinHjelmare changed the title ~~KNX: deprecate config_file~~ Deprecate knx config_file Feb 22, 2021

spacegaier reviewed Feb 23, 2021

View reviewed changes

Comment thread homeassistant/components/knx/__init__.py Outdated

Update homeassistant/components/knx/__init__.py

e5b59cf

Co-authored-by: Philip Allgaier <philip.allgaier@gmx.de>

frenck reviewed Feb 23, 2021

View reviewed changes

Comment thread homeassistant/components/knx/__init__.py Outdated

remove notification, add cv.deprecated again

a608fcf

frenck reviewed Feb 23, 2021

View reviewed changes

farmio and others added 2 commits February 24, 2021 00:36

Update homeassistant/components/knx/__init__.py

6bff0ef

Co-authored-by: Franck Nijhof <frenck@frenck.nl>

remove cv.deprecated again

aa7cbb3

bdraco removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 24, 2021

pvizeli approved these changes Mar 1, 2021

View reviewed changes

pvizeli merged commit dadc99d into home-assistant:dev Mar 1, 2021

farmio mentioned this pull request Mar 1, 2021

KNX remove custom deprecation warnings #47238

Merged

21 tasks

github-actions Bot locked and limited conversation to collaborators Mar 2, 2021

	"The 'config_file' option will soon be deprecated. Please replace it with Home Assistant config schema "
	"The 'config_file' option is deprecated. Please replace it with Home Assistant config schema "

Uh oh!

Conversation

farmio commented Feb 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Feb 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bdraco commented Feb 22, 2021

Uh oh!

elupus commented Feb 22, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farmio commented Mar 1, 2021

Uh oh!

frenck commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

farmio commented Mar 1, 2021

Uh oh!

farmio commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

farmio commented Feb 21, 2021 •

edited

Loading

frenck commented Mar 1, 2021 •

edited

Loading