legrego / homeassistant-elasticsearch

Publish Home-Assistant events to Elasticsearch
https://legrego.github.io/homeassistant-elasticsearch/
MIT License
143 stars 38 forks source link

[1.0] Refactor config migrations and remove config parameters #272

Closed strawgate closed 2 weeks ago

strawgate commented 1 month ago

Fixes: #271 Fixes: https://github.com/legrego/homeassistant-elasticsearch/issues/287 Fixes: https://github.com/legrego/homeassistant-elasticsearch/issues/273

Removes config, removes reading config values from anything other than init (and maybe async_init), and moves all components to explicitly referencing either data or options. Also removes the configuration that stops all tests from running after the first failure. Also fixes ES setup on Linux

github-actions[bot] commented 1 month ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
__init__.py00100% 
elasticsearch
   __init__.py112694%119, 155–156, 176–177, 271
   config_flow.py2222090%165, 198, 328, 365, 379, 438–439, 450–452, 466–467, 471, 510, 516, 556, 631, 640, 674–675
   const.py340100% 
   entity_details.py330100% 
   errors.py28389%51, 60, 67
   es_doc_creator.py193199%322
   es_doc_publisher.py2684085%69–70, 140–141, 180, 183, 186, 191, 208–209, 227–230, 234, 249–250, 266–267, 271, 296, 300, 394, 414, 456, 470, 481, 484, 512–513, 515–516, 518–519, 523–525, 544, 546, 552
   es_gateway.py1442086%94–95, 114, 133–135, 137, 139–140, 142–143, 146–148, 153–154, 158–160, 253
   es_index_manager.py1231191%43, 74, 79, 242–243, 260–261, 266–267, 296–297
   es_integration.py35294%45–46
   es_privilege_check.py600100% 
   es_serializer.py10190%17
   es_version.py300100% 
   logger.py20100% 
   system_info.py25196%37
   utils.py40100% 
TOTAL132310592% 

Tests Skipped Failures Errors Time
222 0 :zzz: 0 :x: 0 :fire: 11.773s :stopwatch:
strawgate commented 1 month ago

@legrego one of the things im thinking about doing in this PR is to stop using build_full_config and to start using build_data and build_options instead to make it explicit where these are coming from as options are currently duplicated between data and options.

This will probably mean I have to touch a lot of tests so I want to check in with you before I do that.

legrego commented 1 month ago

@legrego one of the things im thinking about doing in this PR is to stop using build_full_config and to start using build_data and build_options instead to make it explicit where these are coming from as options are currently duplicated between data and options.

This will probably mean I have to touch a lot of tests so I want to check in with you before I do that.

@strawgate That makes sense to me, I'd be happy for you to do this. Thanks!

strawgate commented 1 month ago

@legrego and maybe drop yaml config? :)

legrego commented 1 month ago

I'm not prepared to drop yml config just yet 🙂. I believe that's still being used in the wild

strawgate commented 1 month ago

I'm not prepared to drop yml config just yet 🙂. I believe that's still being used in the wild

that should be fine though right? doesn't the yml config get imported into data/options on each run?

So the only thing that would break would be users adding new key/values to their yml config?

strawgate commented 1 month ago

As I'm getting started on this I'm also realizing that we could probably stop accepting a config param any place that the config_entry is available? Does that sound right?

legrego commented 1 month ago

I'm not prepared to drop yml config just yet 🙂. I believe that's still being used in the wild

that should be fine though right? doesn't the yml config get imported into data/options on each run?

So the only thing that would break would be users adding new key/values to their yml config?

I think the component will fail to start if there is unexpected yml config in the file. That's how it behaved in the past anyway.

legrego commented 1 month ago

As I'm getting started on this I'm also realizing that we could probably stop accepting a config param any place that the config_entry is available? Does that sound right?

Yes! That's been on my mental todo list for awhile.

strawgate commented 1 month ago

This commit shows my rough plan for removing config and explicitly pulling data and options:

https://github.com/legrego/homeassistant-elasticsearch/pull/272/commits/07508e3f843253405f750de53f3453e6a8cf6a4e

It looks like config was passed in a couple of places to allow things like testing the gateway without committing the settings to the config_entry.

In the case of the gateway I've added some static methods to es_gateway to allow testing connections without "mocking" a config object.

legrego commented 4 weeks ago

This commit shows my rough plan for removing config and explicitly pulling data and options:

07508e3

It looks like config was passed in a couple of places to allow things like testing the gateway without committing the settings to the config_entry.

In the case of the gateway I've added some static methods to es_gateway to allow testing connections without "mocking" a config object.

The general approach LGTM. Is the plan to remove the ESPrivilegeCheck class in favor of the privilege checks you're embedding into the gateway?

strawgate commented 4 weeks ago

I could go either way on ESPrivilegeCheck.

I do think it makes sense for the gateway to have something (generic) where you can pass in a set of permissions and see if the gateway's connection is capable of those permissions. And then just leveraging that functionality when initializing the gateway?

The legacy datastreams needed some fancy variable substitution for dynamic privilege checking, the new datastreams method is a static permission set. A static permission set makes the whole class a bit less exciting I think.

strawgate commented 3 weeks ago

Fixes: https://github.com/legrego/homeassistant-elasticsearch/issues/287 Fixes: https://github.com/legrego/homeassistant-elasticsearch/issues/273

strawgate commented 3 weeks ago

apologies in advance...

strawgate commented 2 weeks ago

Added PR feedback and comments

strawgate commented 2 weeks ago

Addressed / incorporated all feedback

strawgate commented 2 weeks ago

!!!!