search

legrego / homeassistant-elasticsearch

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

Split out async_migrate_entry #271

Closed strawgate closed 2 weeks ago

strawgate commented 1 month ago

Right now all the migration logic is in async_migrate_entry. Each time we update the migration logic we have to go back and modify all existing tests as there is not currently a way to test individual upgrades like v1 -> v2, all tests must be v1 -> latest.

I'd like to split out async_migrate_entry into a function like migrate_config_entry_to_version that takes a config and an ending version and applies the required changes to get to the ending version.

As part of this work I'd also split out config from options and move the migration to using supported config update methods.

Raising this issue to collect feedback before I make a PR

legrego commented 1 month ago

I'd like to split out async_migrate_entry into a function like migrate_config_entry_to_version that takes a config and an ending version and applies the required changes to get to the ending version.

I'm not opposed to breaking this down into isolated, testable functions. We have to keep async_migrate_entry in place, as that is what Home Assistant will invoke automatically on startup (docs). Given that, I'm not sure how much it'll reduce the number of tests that require updating, but I have no objections to a refactor.

As part of this work I'd also split out config from options and move the migration to using supported config update methods.

Yep, the former (#242) and latter (#226) are both important tasks, and I'm happy for you to pick these up if you're interested. I request that the PRs are kept to a reasonable size to make reviews easier. So my preference is a couple of moderate PRs over one large PR, if this ends up turning into a lot of work.

strawgate commented 1 month ago

Here's an example from adding the v4 config entry where I had to modify the tests for v1 and v2 https://github.com/legrego/homeassistant-elasticsearch/pull/207/files#diff-056c6b0b13b9539a120e8832b32e9cf5e681eb230fda35a261267d6381a0599eR256-R289

Essentially I'd like to have tests for the v1/v2/v3 config migrations that don't need to be changed every time we bump the config version as touching the old tests will make it harder to know that we haven't broken something.

legrego commented 1 month ago

Here's an example from adding the v4 config entry where I had to modify the tests for v1 and v2 https://github.com/legrego/homeassistant-elasticsearch/pull/207/files#diff-056c6b0b13b9539a120e8832b32e9cf5e681eb230fda35a261267d6381a0599eR256-R289

Essentially I'd like to have tests for the v1/v2/v3 config migrations that don't need to be changed every time we bump the config version as touching the old tests will make it harder to know that we haven't broken something.

Makes sense to me. I'd like to leave one test at this level which ensures we can run through the entire async_migrate_entry function successfully, but isolating the version-specific tests is a good idea.