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

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

Merged
merged 24 commits into from
May 3, 2024

Conversation

strawgate
Copy link
Collaborator

@strawgate strawgate commented Apr 17, 2024

Fixes: #271
Fixes: #287
Fixes: #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

Copy link
Contributor

github-actions bot commented Apr 17, 2024

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 💤 0 ❌ 0 🔥 11.773s ⏱️

@strawgate
Copy link
Collaborator Author

strawgate commented Apr 17, 2024

@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
Copy link
Owner

legrego commented Apr 17, 2024

@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
Copy link
Collaborator Author

@legrego and maybe drop yaml config? :)

@legrego
Copy link
Owner

legrego commented Apr 18, 2024

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

@strawgate
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

legrego commented Apr 18, 2024

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
Copy link
Owner

legrego commented Apr 18, 2024

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 strawgate changed the title Refactor config migrations Draft: Refactor config migrations Apr 18, 2024
@strawgate
Copy link
Collaborator Author

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.

@strawgate strawgate changed the title Draft: Refactor config migrations Draft: Refactor config migrations and remove config parameters Apr 18, 2024
Add a mock for entity state so we don't need to initialize the entire integration to mess with state objects. Also allow all tests to run if there is a failure (Might break CI?)
@legrego
Copy link
Owner

legrego commented Apr 19, 2024

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
Copy link
Collaborator Author

strawgate commented Apr 19, 2024

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
Copy link
Collaborator Author

Fixes: #287
Fixes: #273

@strawgate strawgate changed the title Draft: Refactor config migrations and remove config parameters Refactor config migrations and remove config parameters Apr 25, 2024
@strawgate
Copy link
Collaborator Author

apologies in advance...

@strawgate strawgate requested a review from legrego April 25, 2024 03:27
Copy link
Owner

@legrego legrego left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thanks for the refactor! The parameterized tests and snapshots are much easier to reason about as well 👏

custom_components/elasticsearch/__init__.py Outdated Show resolved Hide resolved
custom_components/elasticsearch/__init__.py Show resolved Hide resolved
custom_components/elasticsearch/__init__.py Show resolved Hide resolved
custom_components/elasticsearch/__init__.py Show resolved Hide resolved
custom_components/elasticsearch/config_flow.py Outdated Show resolved Hide resolved
custom_components/elasticsearch/config_flow.py Outdated Show resolved Hide resolved
custom_components/elasticsearch/config_flow.py Outdated Show resolved Hide resolved
tests/archive Outdated Show resolved Hide resolved
tests/test_es_doc_publisher.py Show resolved Hide resolved
tests/test_es_doc_publisher.py Outdated Show resolved Hide resolved
@strawgate
Copy link
Collaborator Author

Added PR feedback and comments

@strawgate strawgate changed the title Refactor config migrations and remove config parameters [1.0] Refactor config migrations and remove config parameters May 2, 2024
@strawgate strawgate enabled auto-merge (squash) May 3, 2024 01:33
@strawgate
Copy link
Collaborator Author

Addressed / incorporated all feedback

Copy link
Owner

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM once merge conflict is addressed

@strawgate strawgate merged commit eb56a9a into legrego:main May 3, 2024
3 checks passed
@strawgate
Copy link
Collaborator Author

!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants