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 support for glob matching in InfluxDB filters #37069

Merged
merged 2 commits into from Jun 26, 2020
Merged

Add support for glob matching in InfluxDB filters #37069

merged 2 commits into from Jun 26, 2020

Conversation

mdegat01
Copy link
Contributor

@mdegat01 mdegat01 commented Jun 24, 2020

Breaking change

InfluxDB was not using the common filtering logic shared by recorder, logbook, homekit, etc. and as a result had filtering logic that is inconsistent with the filtering logic of every other recorder-like component. This has been corrected causing the following changes in filtering logic:

  1. Same domain specified in both include and exclude
  • Previous behavior: All entities in that domain excluded
  • New behavior: All entities of that domain included unless entity is excluded by ID or by glob
  1. Same entity ID specified in both include and exclude
  • Previous behavior: Entity excluded
  • New behavior: Entity included
  1. Filtering has 1+ exclude domains, 0 include domains, and 1+ include entity ID's specified:
  • Previous behavior: All entities not specifically listed by ID were excluded
  • New behavior: All entities not specifically excluded by either domain or ID are included

Proposed change

This is a follow-up to my recent PR: #36913. I want to bring glob filtering capability to InfluxDB as well since it is also a recorder-like component with an identical filtering config. Users would have a reasonable expectation that the Influx component has identical behavior to recorder, logbook` and the others when it comes to entity filtering capability. Unfortunately that is not currently the case since Influx does not currently share the common filtering logic and instead has created its own.

This PR therefore makes the following changes:

  1. Bring the new GLOB-filtering capability added to the other recorder-like components to Influx
  2. Correct the inconsistencies in Influx's filtering logic when compared to the other recorder-like components (see breaking changes for differences)

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:

# Example configuration.yaml
influx:
  include:
    domain: sensor
    entity_globs: binary_sensor.*_occupancy
  exclude:
    entity_globs: sensor.weather_*

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

@probot-home-assistant
Copy link

Hey there @fabaff, mind taking a look at this pull request as its been labeled with an integration (influxdb) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@mdegat01 mdegat01 changed the title GLOB filter support for InfluxDB Add support for glob matching to Influx filtering Jun 24, 2020
@mdegat01 mdegat01 changed the title Add support for glob matching to Influx filtering Add support for glob matching to filtering in InfluxDB Jun 24, 2020
@mdegat01 mdegat01 changed the title Add support for glob matching to filtering in InfluxDB Add support for glob matching in InfluxDB filters Jun 24, 2020
@mdegat01
Copy link
Contributor Author

Since I realize changes to existing tests is something which raises flags in a review wanted to pre-emptively explain my reasoning above.

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.

Please remove the refactor that is not related to the filtering from this PR. We should keep the PR scope minimal.

homeassistant/components/influxdb/const.py Outdated Show resolved Hide resolved
homeassistant/components/influxdb/__init__.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Jun 25, 2020
@project-bot project-bot bot moved this from Review in progress to Needs review in Dev Jun 25, 2020
@home-assistant home-assistant deleted a comment from homeassistant Jun 25, 2020
@mdegat01
Copy link
Contributor Author

@MartinHjelmare I'm not gonna lie, that was painful. I really liked my refactor, I cleaned up a lot of stuff. But I get it, keep it small. I put all of the excluded work into a new branch so I can come back to it later. PR is much much smaller now.

@MartinHjelmare
Copy link
Member

I appreciate the work and I think refactoring the integration is a good plan. We should just take it in smaller steps. That will allow us to move faster in total with less risk.

tests/components/influxdb/test_init.py Outdated Show resolved Hide resolved
tests/components/influxdb/test_init.py Outdated Show resolved Hide resolved
@mdegat01
Copy link
Contributor Author

@MartinHjelmare it still says "Changes requested" but that change was to remove the unreleated refactoring changes which is done now right?

Dev automation moved this from Needs review to Reviewer approved Jun 25, 2020
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.

Looks good!

@mdegat01
Copy link
Contributor Author

@MartinHjelmare Can it be merged or is there additional review required? Or is this more that its too late for 0.112 and has to wait for 0.113 to open up?

@MartinHjelmare
Copy link
Member

We can merge. It'll go out in 0.113. 馃憤

@MartinHjelmare MartinHjelmare merged commit d454f85 into home-assistant:dev Jun 26, 2020
Dev automation moved this from Reviewer approved to Done Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants