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

Add missing migration of transport entry points in archives #5604

Conversation

janssenhenning
Copy link
Contributor

Fixes #5587

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @janssenhenning . This would be an important fix to add indeed. The fix itself looks good, however, we can't be putting this in the existing migration and should really rather create a new one. The reason is that since the 11 -> 12 migration has already been released, there will be archives out there with v12 that does not contain the migration. To have these migrate as well, we need to create a new 12 -> 13 migration. Could you add this separately? You can just copy paste from existing migrations and tests

@janssenhenning janssenhenning force-pushed the fix/missing-transport-entry-point-migration branch from e202254 to 46034f7 Compare August 3, 2022 08:21
@janssenhenning
Copy link
Contributor Author

@sphuber Sorry for the late response. I now moved it into a separate migration

@janssenhenning janssenhenning requested a review from sphuber August 3, 2022 08:22
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @janssenhenning that looks good. Could you just add a test for it please? If you don't have the time, let me know and I can see if I can add it, no problem.

@janssenhenning
Copy link
Contributor Author

@sphuber There is a test that the migration works. The test in tools/archive/migration/test_legacy_funcs.py is parametrized with all of the migrations in the dictionary LEGACY_MIGRATE_FUNCTIONS in aiida.storage.sqlite_zip.migrations.legacy

I didin't see an explicit test for the 0.11 to 0.12 migration to check if the entry points are correctly changed beyond the test I mentioned. If I missed it could you point me to the file, where this test is located? I can add this of course if you want.

@sphuber
Copy link
Contributor

sphuber commented Aug 4, 2022

@sphuber There is a test that the migration works. The test in tools/archive/migration/test_legacy_funcs.py is parametrized with all of the migrations in the dictionary LEGACY_MIGRATE_FUNCTIONS in aiida.storage.sqlite_zip.migrations.legacy

That parametrized test just checks that the migration does not except and the generic migration actions, such as updating the version in the metadata, is done correctly. There is no explicit check on the actual migration specific functionality.

I didin't see an explicit test for the 0.11 to 0.12 migration to check if the entry points are correctly changed beyond the test I mentioned. If I missed it could you point me to the file, where this test is located? I can add this of course if you want.

You are right, there isn't one for 0.11 to 0.12. There are a few more missing for recent migrations, but they are there uptil 0.9. They are in the directory tools/archive/migration/. Would be good to add test_v012_to_v013.py there.

@janssenhenning janssenhenning force-pushed the fix/missing-transport-entry-point-migration branch from c48f033 to 3568894 Compare August 4, 2022 11:29
@janssenhenning
Copy link
Contributor Author

Would be good to add test_v012_to_v013.py there.

Done

@janssenhenning janssenhenning requested a review from sphuber August 4, 2022 12:07
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @janssenhenning !

@sphuber sphuber merged commit 3e4c883 into aiidateam:main Aug 4, 2022
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 this pull request may close these issues.

Archive: Missing migration of transport entry point for Computer
2 participants