Create backup of configuration file before migrating #3568
Conversation
|
@giovannipizzi Wanted to include this in a |
|
I guess you meant |
|
Yes, I meant |
sphuber
force-pushed
the
fix_3510_config_migration_backup
branch
from
7345a6f
to
275aaa8
Compare
aiida/manage/configuration/config.py
Outdated
| version = get_current_version(config_dictionary) | ||
| filepath_backup = '{}.v{}.{}'.format(filepath, version, timezone.now().strftime('%Y%m%d-%H%M%S')) | ||
|
|
||
| echo.echo_warning('current configuration file `{}` is outdated and will be migrated'.format(filepath)) |
There was a problem hiding this comment.
To me, these two echo statements don't belong in this function - the function should just create the backup, right?
I would move them to where you do the migration
There was a problem hiding this comment.
I have looked at the code a bit better and think I have found a solution that is a lot better and aligns with your suggestion. Construction of Config from file is now done through Config.from_file class method which is now responsible for migrating and creating of backup. This bundles these two operations more closely, and for the better I feel.
aiida/manage/configuration/config.py
Outdated
| from aiida.cmdline.utils import echo | ||
|
|
||
| version = get_current_version(config_dictionary) | ||
| filepath_backup = '{}.v{}.{}'.format(filepath, version, timezone.now().strftime('%Y%m%d-%H%M%S')) |
There was a problem hiding this comment.
Maybe switch the order of version and date so that the files will be ordered by date.
Also, we already have a backup function that creates backups of the config file ending in .json~
Perhaps we should be reusing this function.
There was a problem hiding this comment.
Can't do because that is a member function of Config which in order to be constructed requires a fully migrated config dictionary, so at that point creating a backup will be too late. It would be possible if we move the logic that determines whether migration is necessary in the constructor of the Config class. I am not sure if that is better. The code there is already a bit complicated and might make it even more tightly coupled. On the other hand maybe it makes it more self-contained and therefore better. I will take a look what that would look like.
There was a problem hiding this comment.
Also re the ordering, with the current order they will still be ordered by data, within each version group, which should also be ordered, no?
There was a problem hiding this comment.
Decided to omit the version as it is in the files themselves.
sphuber
force-pushed
the
fix_3510_config_migration_backup
branch
from
9675592
to
1a8b4f1
Compare
This allows users to revert in case of problems with the migration or if the migration happens by accident. Since the migration is done automatically without prompt and is not reversible, having a backup is crucial if users checkout a version of the code by accident. To accomplish this the configuration instantiation and migration is also refactored moving the responsibility of migrating a configuration file to the `Config` class itself. Constructing a `Config` instance from an existing configuration file should now be done through the `from_file` class method. This will backup the file if it determines that it is outdatedand then construct a `Config` instance from the migrated contents.
sphuber
force-pushed
the
fix_3510_config_migration_backup
branch
from
1a8b4f1
to
55861f5
Compare
It is actually necessary without concurrent backups. If you look in the constructor there are multiple checks for inconsistencies and for each one it will create a backup. These can follow one another pretty quickly so having a granularity of seconds ran the risk of the first backup with the original file being overridden. |
Fixes #3510
This allows users to revert in case of problems with the migration or if
the migration happens by accident. Since the migration is done
automatically without prompt and is not reversible, having a backup is
crucial if users checkout a version of the code by accident.
P.S.: tests not included.