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

Scripts for local file migrations. Closes #642. #648

Merged
merged 14 commits into from
Apr 6, 2022

Conversation

Jamie-SA
Copy link
Contributor

@Jamie-SA Jamie-SA commented Mar 25, 2022

Migration scripts from v10.0 to v11.0

Closes #642.

I think I covered what we can in the queries
I wrote a very simple test file in ./input/ to be able to do some testing.

Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

You can also do an action_rename on:

hasOrderedMember -> hasMember.

It might be worth adding in the detect_removed error message that we will be providing sample triples to illustrate the model changes and how they will need to change their data.

Otherwise looks good to me.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 4, 2022

@Jamie-SA Since you wrote this we have also deleted baseConversionFactor and standardConversionFactor and use conversionFactor for both. Even though we have a convergence of two predicates, we can include the transformations in the migration scripts. Could you please add this?

@rjyounes rjyounes changed the title WIP: Scripts for local file migrations WIP: Scripts for local file migrations. Closes #642. Apr 4, 2022
@rjyounes rjyounes marked this pull request as draft April 4, 2022 15:41
@rjyounes rjyounes marked this pull request as ready for review April 4, 2022 15:42
@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Apr 4, 2022

I've still got a couple more things to do on this PR, but have to switch gears right now and come back to it later.

@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Apr 5, 2022

It might be worth adding in the detect_removed error message that we will be providing sample triples to illustrate the model changes and how they will need to change their data.

I'm not sure what specifically you were referencing. Try to make suggestions for everything that changed?
That sounds like something that should be in the release notes.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 5, 2022

Yeah, never mind that last comment.

Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

Did you add the renaming of the conversion factor predicates I mentioned? I don't see it. Both standardConversionFactor and baseConversionFactor are renamed to conversionFactor.

Once that's done, I think these are good to go unless you want to do anything else.

@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Apr 5, 2022

Sorry, missed the conversion factor renaming. I've pushed another change to include that.

@Jamie-SA Jamie-SA changed the title WIP: Scripts for local file migrations. Closes #642. Scripts for local file migrations. Closes #642. Apr 5, 2022
@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Apr 5, 2022

I think I am done now. Please review.

@Jamie-SA Jamie-SA requested review from rjyounes and bolerio April 5, 2022 19:44
Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

This looks good to me. The only thing I would add to the readme is that the migrations scripts are not cumulative - I.e. someone on v9 must upgrade to v10 first and then v11. (That's what I was trying to get at when I suggested mentioning v10, but this is a more generic way to say it.)

@rjyounes rjyounes self-requested a review April 6, 2022 17:52
@rjyounes
Copy link
Collaborator

rjyounes commented Apr 6, 2022

@bolerio We are aiming to get this merged by EOD today. Will you have a chance to review it before then?

@Jamie-SA
Copy link
Contributor Author

Jamie-SA commented Apr 6, 2022

I made a couple more changes to address RY's last comment.

Copy link
Contributor

@pwin pwin left a comment

Choose a reason for hiding this comment

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

Looks good

@rjyounes rjyounes merged commit 0f0b246 into develop Apr 6, 2022
@rjyounes rjyounes deleted the feature/issue_642_gist11.0_migration_scripts branch April 6, 2022 21:00
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.

Migration scripts for deletions and name changes in gist 11
3 participants