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

Warn users to migrate when old schema is detected and enable migration CLI #816

Merged
merged 8 commits into from Nov 6, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Sep 18, 2022

Description

This PR adds a check in project initialization to ensure that projects with old schemas are detected when signac fails to find a project with the current schema. The resulting check provides the user a meaningful error about how to proceed with updating their schema using our migration code. This PR also exposes a CLI for migration to complement the Python interface, providing a simpler and friendlier approach to one-off migration that is recommended by the error.

Motivation and Context

Necessary to for projects how to migrate to v2.

Checklist:

@vyasr vyasr added this to the v2.0.0 milestone Sep 18, 2022
@vyasr vyasr self-assigned this Sep 18, 2022
@vyasr vyasr requested review from a team as code owners September 18, 2022 19:51
@vyasr vyasr requested review from mikemhenry and SchoeniPhlippsn and removed request for a team September 18, 2022 19:51
@codecov
Copy link

codecov bot commented Sep 18, 2022

Codecov Report

Merging #816 (a87e785) into master (9aa65c0) will increase coverage by 0.12%.
The diff coverage is 88.63%.

@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
+ Coverage   85.62%   85.75%   +0.12%     
==========================================
  Files          51       51              
  Lines        4641     4683      +42     
  Branches      917      923       +6     
==========================================
+ Hits         3974     4016      +42     
+ Misses        481      480       -1     
- Partials      186      187       +1     
Impacted Files Coverage Δ
signac/__main__.py 77.79% <68.75%> (-0.22%) ⬇️
signac/common/config.py 87.67% <100.00%> (+4.33%) ⬆️
signac/contrib/migration/v0_to_v1.py 86.66% <100.00%> (+3.33%) ⬆️
signac/contrib/migration/v1_to_v2.py 90.00% <100.00%> (+0.81%) ⬆️
signac/contrib/project.py 89.84% <100.00%> (+0.19%) ⬆️
signac/contrib/migration/__init__.py 93.10% <0.00%> (+6.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I haven't tested this locally but I'm happy with the code in this PR! I'll test it with v1 projects on the next branch sometime in the near future.

signac/__main__.py Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Sep 19, 2022

I haven't tested this locally but I'm happy with the code in this PR! I'll test it with v1 projects on the next branch sometime in the near future.

I'll hold off on merging to let you try a test or two in case you catch bugs that I missed in my testing.

signac/__main__.py Outdated Show resolved Hide resolved

try:
schema_version = _get_config_schema_version(root, schema_version)
assert schema_version != int(SCHEMA_VERSION), (
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an if is better here? This is one of those nit picky things but since assert isn't ran with -O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I sort of did that intentionally since this indicates developer error somewhere. This is really intended to be something more like a debug print. I can remove it or change to an if statement if you'd prefer though.

@bdice
Copy link
Member

bdice commented Oct 20, 2022

@vyasr Maybe we should wait until I rebase next and then rebase this PR?

@vyasr
Copy link
Contributor Author

vyasr commented Oct 20, 2022

@vyasr Maybe we should wait until I rebase next and then rebase this PR?

Yes that was my plan. I was also hoping to get one or two additional testers aside from myself, but I think once the rebase is done if nobody has done additional testing at that point we can just go ahead and merge this and let people test it on the main branch.

@vyasr vyasr changed the base branch from next to master November 2, 2022 03:39
@vyasr
Copy link
Contributor Author

vyasr commented Nov 2, 2022

@bdice @mikemhenry if one of you would like to test this locally that would be great, otherwise I think we can go ahead and just merge this.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 6, 2022

I'm going to go ahead and merge this. People can test it out on the master branch.

@vyasr vyasr enabled auto-merge (squash) November 6, 2022 15:30
@vyasr vyasr merged commit db2b9fa into master Nov 6, 2022
@vyasr vyasr deleted the feature/warn_migration branch November 6, 2022 15:36
@cbkerr cbkerr mentioned this pull request Mar 29, 2023
6 tasks
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.

None yet

4 participants