[DA] Update default AutomationConditionSensorDefinition name #23017
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @OwenKephart and the rest of your teammates on |
347e19c
to
0f5b4ad
Compare
0f5b4ad
to
3ed5d7c
Compare
26454bb
to
60844d5
Compare
3ed5d7c
to
b5bbe50
Compare
| @@ -626,6 +653,43 @@ def _create_initial_sensor_cursors_from_raw_cursor( | |||
|
|
|||
| return result | |||
|
|
|||
| def _rename_auto_materialize_states( | |||
There was a problem hiding this comment.
this is more of a copy than a rename right? (which is good in the hopefully unlikely case that a user needs to roll back - the old state with the old name will still be there, harmless)
There was a problem hiding this comment.
that's correct yeah -- changed the comment here to be a bit more accurate and updated the name to _copy_default_auto_materialize_sensor_states.
| @@ -509,8 +524,20 @@ def _run_iteration_impl( | |||
| ) | |||
|
|
|||
| set_has_migrated_to_sensors(instance) | |||
| if not get_has_migrated_sensor_names(instance): | |||
There was a problem hiding this comment.
just confirming my understanding - for new users who have never used AMP before or had a sensor before, the only thing this does is set a key in the kvs table right? Nothing gets moved or migrated in that case?
There was a problem hiding this comment.
that's correct yep -- it will not find any historical instigator data in the database and so no migration is actually happening
| for sensor, repo in sensors_and_repos: | ||
| # find the corresponding sensor with the updated name | ||
| if ( | ||
| sensor.name == "default_automation_condition_sensor" | ||
| and repo.get_external_origin() == instigator_state_repository_origin | ||
| ): | ||
| new_auto_materialize_state = InstigatorState( | ||
| sensor.get_external_origin(), |
There was a problem hiding this comment.
i am a little unsure why we need sensors_and_repos here at all - can the new origin to use not always be derived from the old origin?
i.e. could this not be
new_sensor_origin = instigator_state.origin._replace(instigator_name="default_automation_condition_sensor")
and then we create it, whether or not there is a sensor with that name?
Not sure if it matters in practice but seems a bit simpler
There was a problem hiding this comment.
Yeah the main thing here was just having access to the selector id, which given your comment below is actually possible to get without the sensor itself. This seems a lot better, and guards against a case where the given code location is unavailable at the time of the migration
| instigator_state.status, | ||
| instigator_state.instigator_data, | ||
| ) | ||
| result[sensor.selector_id] = new_auto_materialize_state |
There was a problem hiding this comment.
if you took out sensors_and_repos this would then become
result[new_sensor_origin.get_selector().get_id()]
24c9ebd
to
c5fbcc3
Compare
60844d5
to
3e6b391
Compare
c5fbcc3
to
35e2fec
Compare
Merge activity
|
3e6b391
to
2432442
Compare
35e2fec
to
d6792f7
Compare
Summary & Motivation
Creates a one-time migration to handle the fact that we're updating the default sensor name. In these cases, we want to make sure that we preserve the history of these default sensors (particularly their cursors) so that this change is not disruptive.
Still working on creating tests.
How I Tested These Changes