-
Notifications
You must be signed in to change notification settings - Fork 501
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
bump to Postgres 17, Flyway 10.19, allows earlier versions of PostgreSQL #10912
Conversation
@donsizemore IIRC you needed to add a Postgres Package for Flyway with 10.x. Is this no longer true? |
@poikilotherm I thought that was this? https://github.com/IQSS/dataverse/pull/10912/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R182 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great overall. I left a couple comments.
As a sanity check, I went to https://github.com/gdcc/dataverse-ansible/blob/51587e7231ffca474ad2b751f2e4508f7f9f8c7d/tests/group_vars/jenkins.yml#L277 which is the latest, to see that we're still going to test with Postgres 13. That's great because we still say "PostgreSQL 13 is recommended because it’s the version we test against" as of this PR at https://dataverse-guide--10912.org.readthedocs.build/en/10912/installation/prerequisites.html#postgresql The docs seem fine and accurate as-is. This PR will have the effect of upgrading the Docker images to Postgres 17. I'll go add a suggestion to the release notes about that. It will mean the Docker images will be out of step with the classic installation. This is probably ok as the Docker image is already at 16 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new suggestion.
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
|
||
While we don't encourage the use of older Postgres versions, this flexibility may benefit some of our long-standing installations in their upgrade paths. Postgres 13 remains the version used with automated testing. | ||
|
||
As part of this update, the containerized development environment now uses Postgres 17 instead of 16. Worst case, developers can start with a fresh database, if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious what would happen if I do the following:
- Spin up containers on the develop branch (dev compose file), let Dataverse configure itself
- Stop containers (don't delete data)
- Switch to this branch
- Spin up containers
I got these errors:
dev_dataverse> 2024-11-01T19:06:18Z INF [TCP] Checking the postgres:5432 ...
dev_dataverse> 2024-11-01T19:06:18Z ERR Expectation failed error="failed to establish a tcp connection, caused by: dial tcp: lookup postgres on 127.0.0.11:53: no such host"
Next I'll try blowing the data away.
Time passes...
Yes, that worked fine. I rm -rf'd docker-dev-volumes and everything spun up fine.
I think we need to tell developers they need to blow away their data. I'll add a suggestion to the release note (and when the time comes, we should email the dev list).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure developers need to blow away their database, so I added a suggestion to the release note for that.
I also tested the demo use case (spin up a new demo env) and it worked fine. PG 17 is correctly used:
pdurbin@beamish dataverse % docker exec -it postgres bash
root@postgres:/# export PGPASSWORD=secret
root@postgres:/# psql -h localhost -U dataverse dataverse
psql (17.0 (Debian 17.0-1.pgdg120+1))
describe zapping database in Docker environment Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and all API tests are passing: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10912/11/testReport/
Performed regression testing on Internal. No issues uncovered - Merging PR. postgres.mov |
I sent a note about this: https://groups.google.com/g/dataverse-dev/c/ffoNj5UXyzU/m/nE5oGY_sAQAJ |
What this PR does / why we need it:
Bumps Postgres to version 17 and Flyway to version 10.19. The Flyway version change is significant, as version 10 removes the requirement that users purchase licenses to use older/non-supported versions of Postgres.
Which issue(s) this PR closes:
Special notes for your reviewer:
None
Suggestions on how to test this:
Standard test suite run.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No.
Is there a release notes update needed for this change?:
Might be nice to note the removal of the Flyway restriction? Don't want to encourage older versions, tho.
Additional documentation:
None