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

Migrations can't remove settings #641

Closed
tjkirch opened this issue Jan 13, 2020 · 3 comments · Fixed by #644
Closed

Migrations can't remove settings #641

tjkirch opened this issue Jan 13, 2020 · 3 comments · Fixed by #644
Assignees

Comments

@tjkirch
Copy link
Contributor

tjkirch commented Jan 13, 2020

The migrator works by:

  1. Copying the data store.
  2. Reading the data into a Settings-like structure in memory.
  3. Letting the migration do what it wants to that structure.
  4. Writing the structure out, key by key, to the copy on disk.

You can remove a setting from the structure, but because it already exists in the copied data store, it will still be there. Keys are only overwritten or added.

@tjkirch tjkirch self-assigned this Jan 13, 2020
@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 13, 2020

My thoughts on options are:

  • Don't copy the whole data store, only the top-level structure of it, so that when we write out the modified data, it writes only what the migration intended to be there.
  • Change the interface of migrations so they specify what's added/changed and what's removed.

Leaning heavily toward the first option; I'll try it out.

@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 20, 2020

I did go with the first option in #644, but it means a change in the interface between migrator and migrations. The new migrator needs to be able to run existing migrations so that we can upgrade. The old migrator needs to be able to run new migrations, at least to a point - we could choose not to support downgrades below some version, after a time.

I think the best thing to do is update the migration-helpers library (which all existing migrations use) to support both interfaces. It would ensure it was only given the old arguments or the new arguments. If given the new arguments (source and target data store) it would read from the source and write to the target. If given the old arguments (single data store) it would set source and target to that single data store; this would mean the migration wouldn't be able to remove any keys, but we know that migrations before this point didn't have to. Then we'd rebuild all existing migrations against the new library, and replace them in the repo.

The special case is the new migration for region, which has to remove a key on downgrade, and any similar future migrations that need to remove a key on downgrade. As long as we want to support downgrading to versions with the old migration interface, they have to be able to remove keys even with the old interface. The only way to do that is removing files on disk directly. We could either (a) make a special function in migration-helpers that migrations need to know to call if they want to remove a key (before the old interface is deprecated), or (b) have migration-helpers diff the input and output of the migration to find anything it removed, so it can remove the file without the migration needing to say to.

I think choosing (a) or (b) depends on the difficulty of making (b) and how long we want to support old versions of the OS without the new migration interface.

@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 20, 2020

Ah, I forgot that the input/output format is just a HashMap, and not a structured Settings-like object, so it's easy to see what was removed. I'll try (b).

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

Successfully merging a pull request may close this issue.

1 participant