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

Centralize knx config and update xknx to 0.12.0 #39219

Merged
merged 2 commits into from
Aug 26, 2020

Conversation

marvin-w
Copy link
Contributor

@marvin-w marvin-w commented Aug 24, 2020

Breaking change

This change incorporates the following pull requests that have been merged to an intermediary branch in order to be able to merge them together to dev once done.

Proposed change

  • Refactor the xknx integration in order to comply with new configuration schema (ADR007).
  • Update xknx to 0.12.0.

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

Example entry for configuration.yaml:

knx:
  tunneling: 
    host: '192.168.0.1'
  cover: !include knx_cover.yaml
  climate: !include knx_climate.yaml
  light: !include knx_light.yaml
  binary_sensor: !include knx_binary_sensor.yaml
  switch: !include switch.yaml
  sensor: !include sensor.yaml
  notify: !include notify.yaml

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

@probot-home-assistant
Copy link

Hey there @Julius2342, 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)

@marvin-w marvin-w marked this pull request as ready for review August 26, 2020 07:48
@marvin-w
Copy link
Contributor Author

marvin-w commented Aug 26, 2020

Can you please restart the CI pipeline? The error was unrelated to that change (and green in the original PR (#38880))

@marvin-w
Copy link
Contributor Author

marvin-w commented Aug 26, 2020

Thank you @frenck! There still appears to be an error with the cached virtual environments:

image

Cache not found for input keys: Linux-venv-3.7-1715f2d5fa43055d4c9f601445d0c767ac64822d152e8c870b198f0bb63980f4-5c90b0c1eb0904cdcc41364933d33f3a76986c9ebb6b665cbbd4a8efc97382f7-e9a553241391d0a2a8ad4725137d163e1e9a5664b55a5a27760765c447290717

@frenck
Copy link
Member

frenck commented Aug 26, 2020

I have no clue 🤷 Could you rebase your PR onto the latest dev branch please?

@marvin-w
Copy link
Contributor Author

@frenck This PR is just a wrapper for two other PRs that have already been reviewed ( #39189 & #38880) and merged into an intermediary branch within HA. If I rebase onto the latest dev I'm effectively rewriting the history (and thus a force push is needed - for which I do not have sufficient permission).

@MartinHjelmare created the branch within HA yesterday. I could merge fast forward but since you specifically asked for a rebase I wanted to clarify first.

@frenck frenck force-pushed the xknx-refactor-updates-0.12.0 branch from 1896b9a to f28aeaf Compare August 26, 2020 12:34
@frenck
Copy link
Member

frenck commented Aug 26, 2020

Rebased it.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare MartinHjelmare changed the title Updates xknx to 0.12.0 and refactores the xknx integration in order to comply with new configuration schema (ADR007) Centralize knx config and update xknx to 0.12.0 Aug 26, 2020
@MartinHjelmare MartinHjelmare merged commit a265184 into dev Aug 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the xknx-refactor-updates-0.12.0 branch August 26, 2020 16:03
weissm pushed a commit to weissm/core that referenced this pull request Aug 28, 2020
* Refactor KNX integration to centralize configuration yaml (home-assistant#39189)

* Updates for xknx 0.12.0 (home-assistant#38880)
leikoilja pushed a commit to leikoilja/core that referenced this pull request Sep 6, 2020
* Refactor KNX integration to centralize configuration yaml (home-assistant#39189)

* Updates for xknx 0.12.0 (home-assistant#38880)
@SquaredCircleHunter
Copy link

SquaredCircleHunter commented Sep 27, 2020

It would be a nice addition to also allowing labels for knx includes

Analogous example to the one here:

knx:
  tunneling: 
    host: '192.168.0.1'
  light groups: !include knx-light-groups.yaml
  light switches: !include knx-light-switches.yaml

@MartinHjelmare
Copy link
Member

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

Merged PRs should not be used for enhancement discussion or bug reports.

Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Sep 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.