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

Fix some typos in migration scripts #6149

Closed
wants to merge 1 commit into from
Closed

Conversation

stweil
Copy link
Member

@stweil stweil commented Jul 25, 2024

No description provided.

@henning-gerhardt
Copy link
Collaborator

This will break every installation which is maintaining their database with FlyWay:

[ERROR] Failed to execute goal org.flywaydb:flyway-maven-plugin:8.4.4:validate (default-cli) on project kitodo-data-management: org.flywaydb.core.api.exception.FlywayValidateException: Validate failed: Migrations have failed validation
[ERROR] Migration description mismatch for migration version 2.19
[ERROR] -> Applied to database : Set properies to unique not null
[ERROR] -> Resolved locally    : Set properties to unique not null. Either revert the changes to the migration, or run repair to update the schema history.
[ERROR] Migration checksum mismatch for migration version 2.61
[ERROR] -> Applied to database : 1769151973
[ERROR] -> Resolved locally    : 1654358955. Either revert the changes to the migration, or run repair to update the schema history.
[ERROR] Migration checksum mismatch for migration version 2.68
[ERROR] -> Applied to database : 1266171681
[ERROR] -> Resolved locally    : -964385541. Either revert the changes to the migration, or run repair to update the schema history.
[ERROR] Migration checksum mismatch for migration version 2.71
[ERROR] -> Applied to database : -1061746512
[ERROR] -> Resolved locally    : 3664929. Either revert the changes to the migration, or run repair to update the schema history.

@stweil
Copy link
Member Author

stweil commented Jul 25, 2024

There were already renames and other changes made for the migration scripts in the past as far as I know. Therefore I thought that running repair is something which was accepted.

@stweil
Copy link
Member Author

stweil commented Jul 25, 2024

For our own production database the existing scripts also produce checksum mismatches. So the documentation for migrations should include this case.

@stweil
Copy link
Member Author

stweil commented Jul 25, 2024

Unrelated: I wonder why even new migration scripts are named V2_*.sql. Are they really still related to Kitodo.Production 2.x? Or what is the meaning of "V2"?

@henning-gerhardt
Copy link
Collaborator

Unrelated: I wonder why even new migration scripts are named V2_*.sql. Are they really still related to Kitodo.Production 2.x? Or what is the meaning of "V2"?

V2 has nothing to do with Kitodo.Production 2.

V1 is the base for all upcoming migrations while developing for Kitodo.Production 3.x was ongoing until now and their version begins with V2 prefix.

@henning-gerhardt
Copy link
Collaborator

There were already renames and other changes made for the migration scripts in the past as far as I know. Therefore I thought that running repair is something which was accepted.

Yes this was done in the past and I was not happy over this changes as I maintain a lot of databases which must all adjusted when the databases get updated to a new version usage. Now it is not only a typo correction inside the files now even a migration file get renamed. The typo's may be ugly but this should be discovered while the pull request is reviewed and not a lot of time later and even after the file is part of a release. In my opinion this typo's should stay and not get fixed.

@solth
Copy link
Member

solth commented Jul 29, 2024

I agree with @henning-gerhardt that fixing these typos in comments (!) is not worth creating additional hassle and workloads for administrators, especially when those typos do not really pose any problems. I will therefor not merge the pull request and close it instead.

@solth solth closed this Jul 29, 2024
@stweil
Copy link
Member Author

stweil commented Jul 29, 2024

Is there additional hassle? I have to run flyway:repair anyway and see no additional workloads.

@stweil
Copy link
Member Author

stweil commented Jul 29, 2024

@solth, issue #6139 might require changes which are not in comments and which also require flyway:repair. Therefore I think that fixing typos in comments can be done without any additional hassle and workloads.

@henning-gerhardt
Copy link
Collaborator

@solth, issue #6139 might require changes which are not in comments and which also require flyway:repair. Therefore I think that fixing typos in comments can be done without any additional hassle and workloads.

The difference is: in this pr only "comments" are changed which did not influence the migration itself. In Issue #6139 may the migration itself need be changed (if this is really needed on MacOS) which would be not nice but more understandable.

Using mvn flyway:repair did only "fixing" the entries inside the flyway database table without running the migration script itself which may or not lead to different database schema's if the changed migration file did something total different in its latest version.

@stweil stweil deleted the migration branch July 29, 2024 12:36
@stweil
Copy link
Member Author

stweil commented Dec 1, 2024

Meanwhile the flyway migration produces lots of warnings for migration scripts which seem to use SQL which will no longer be supported in future versions of MariaDB. So sooner or later there will be a hard requirement for updated migration scripts. Then minor fixes like the ones here can be applied without causing "additional hassle".

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.

3 participants