Flyway integration issue - #5344#5349
Conversation
# Conflicts: # doc/sphinx-guides/source/developers/sql-upgrade-scripts.rst
Also make the "diff" as small as possible.
|
|
||
| @PostConstruct | ||
| void migrateDatabase() { | ||
|
|
There was a problem hiding this comment.
IMHO a check on dataSource == null should be done here or a try/catch for NPEs, so the user gets a reasonable error message. That should finally result in an EJBException getting thrown, so deployment fails.
| import javax.sql.DataSource; | ||
|
|
||
| @Startup | ||
| @Singleton |
There was a problem hiding this comment.
While reading into Flyway in a Java EE context, I stumbled over the annotation
@TransactionManagement(value = TransactionManagementType.BEAN).
The explanation sounds reasonable - should this be added here?
| We assume you have already read the :doc:`version-control` section and have been keeping your feature branch up to date with the "develop" branch. | ||
|
|
||
| Please note that we are aware of the problem of merge conflicts in the SQL update script as well as how the next version number can change at any time. Please see the :doc:`making-releases` section for how we are running an experiment having to do with release notes that might help inform an improvement of our process for developing SQL upgrade scripts. | ||
| First, check https://github.com/IQSS/dataverse/tree/develop/src/main/resources/db/migration to see if a SQL numbers are not colliding with your's. |
There was a problem hiding this comment.
IMHO we could opt for versions based on VYYYY.MM.DD.IDX instead to avoid this for the majority of cases?.
There was a problem hiding this comment.
I think it should be responsibility of the reviewer, there could be even more mess with this system, E.g. user makes PR in 2019.02.15 with same sql name (with id 1) it was merged in 2019.02.20, now user makes pr in 2019.02.16 (with id 1) it was merged in 2019.02.19. Now flyway will crash because it was out of order and not only you need to track id you also need to track dates now.
| ----------------------------------------------- | ||
|
|
||
| ``scripts/database/upgrades`` is the directory where we keep or SQL upgrade scripts. | ||
| ``scripts/database/upgrades`` is the directory where we keep SQL upgrade scripts - legacy way of upgrading schema. |
There was a problem hiding this comment.
Couldn't those be migrated into Flyway someway, too? It would be really cool to get people currently on older releases onto the latest release as easy as possible.
There was a problem hiding this comment.
I think we should stick with the old way till the user is in the newest version, trying to be back-compatible IMO introduces too much mess, you would have to came up with some way to pass the previous app version and then make migration according to that. And the script which we have already does that.
|
|
||
| How to Create or Update a SQL Upgrade Script | ||
| -------------------------------------------- | ||
| If you are doing anything other than creating a new database table such as adding a column to an existing table, you must create SQL. |
There was a problem hiding this comment.
Maybe we should place a hint here that adding an attribute to an @Entity annotated class is just the same?
Maybe a practical example is usefull here, too.
There was a problem hiding this comment.
I'm not exactly sure what do you mean by the same, adding a field in @Entity annotated class without any SQL script wouldn't work since our DDL Generetion is set to create-tables only.
pom.xml
Outdated
| <!-- https://stackoverflow.com/questions/46177921/how-to-run-unit-tests-in-excludedgroups-in-maven --> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <version>2.22.0</version> | ||
| <version>3.0.0-M1</version> |
There was a problem hiding this comment.
I can't find good resources about what might be affected by this version change, as this is really new. The official release notes just show a few improvements/fixes, so everything should be just fine.
Dunno what IQSS policy is on this. You could upgrade to 2.22.1, too.
There was a problem hiding this comment.
The reason for the upgrade is that the maven build was not passing on my pc, after reaserch I found out that it was realted to OpenJDK 8 bug for Debian and at the same time some bug in the surefire plugin which was exactly fixed in this version.
There was a problem hiding this comment.
This looks promising overall. If it works as advertised - if we can have our database upgrades happen automatically, at a price of a couple of extra dependencies, and a small change in how we name the upgrade scripts, it seems reasonable.
But, I'm still trying to understand how it's going to work when upgrading from the releases prior to the release in which we add flywheel to the application. I'm assuming we'll rename all the past upgrade scripts to follow the flywheel convention... But, how does it work exactly when you try to upgrade skipping multiple releases; specifically - how does flywheel know what version you are upgrading from?
|
|
||
| The database schema for Dataverse is constantly evolving. As other developers make changes to the database schema you will need to keep up with these changes to have your development environment in working order. Additionally, as you make changes to the database schema, you must write SQL upgrade scripts when needed and communicate with your fellow developers about applying those scripts. | ||
| The database schema for Dataverse is constantly evolving. As other developers make changes to the database schema you will need to keep up with these changes to have your development environment in working order. Additionally, as you make changes to the database schema, you must write SQL when needed and communicate with your fellow developers about making those SQLs. | ||
|
|
There was a problem hiding this comment.
This may be nitpicking, but I don't fully get why it is necessary to change "upgrade scripts" and "scripts" to "SQLs". I don't see how it makes the text any better; and it's not even a proper use of the word.
?
(I can understand if you feel it is important to have the word "SQL" in there; rather than to just say "script" or "upgrade script". Still, it should probably be "SQL script" and/or "SQL upgrade script", etc. then.)
|
@landreev From my perspective the user first has to upgrade to the version prior to flyway with sql scripts. With new version flyway will do "baseline" on existing db schema and after that you can just add sql scripts. |
6 participants
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist