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

Upgrade to Postgres 14, change client library setup (#381) #382

Merged

Conversation

ssciolla
Copy link
Contributor

This PR aims to resolve #381.

@ssciolla ssciolla marked this pull request as ready for review March 1, 2023 15:10
@ssciolla ssciolla requested a review from zqian March 1, 2023 15:10
@ssciolla ssciolla mentioned this pull request Mar 2, 2023
9 tasks
@jonespm
Copy link
Member

jonespm commented Mar 2, 2023

This looks good. Did you change from the binary to source because of the installation notes or some other reason? Does it add much time to the compilation?

Would you know why these packages are separate and not just in the requirements.txt? Only reason I can think of is so they'll be cached in a separate layer.

@@ -22,10 +22,11 @@ services:
- webpack_watcher

database:
image: postgres:10.4-alpine
image: postgres:14.4-alpine
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to move off of alpine but we aren't changing anything and it isn't used in the deployment, I think it's fine to stay on alpine here. But if we ever upgrade the node dependency in the Dockerfile, I'd switch that over to the node:##-slim tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the downside to alpine? Just curious.

@ssciolla
Copy link
Contributor Author

ssciolla commented Mar 2, 2023

This looks good. Did you change from the binary to source because of the installation notes or some other reason? Does it add much time to the compilation?

@jonespm, I was having issues with the binary file after the upgrade. I thought it might be because of M1, but I'm not sure. Do you have time to change it back and remove the the linux package to see what happens? I'd be curious if you also see issues. It does seem to be recommended that you use this approach, but this isn't something I'm very knowledgeable about.

Would you know why these packages are separate and not just in the requirements.txt? Only reason I can think of is so they'll be cached in a separate layer.

I asked James about this a while ago; I think his rationale was that the database and server libraries shouldn't be coupled to the rest of the application, since some of them could be swapped out or a different Dockerfile could be written. At least that's my recollection.

@jonespm
Copy link
Member

jonespm commented Mar 2, 2023

That makes sense the binary may not work.

I also noticed that the psycopg package has a newer version released at 3.1.8. This 2.9.5 is from 10/2022.

They have a slightly different way of installing this, either as
pip install "psycopg[binary]"
or
pip install "psycopg[c]"

Perhaps this is more of an upgrade for when we actually do upgrade the dependencies in #383

@ssciolla
Copy link
Contributor Author

ssciolla commented Mar 2, 2023

@jonespm, Django says to use psycopg2: https://docs.djangoproject.com/en/3.2/ref/databases/#postgresql-notes

@jonespm
Copy link
Member

jonespm commented Mar 2, 2023

I see, looks like it's going to be supported in Django 4.2, but there was a lot of work involved.

django/django#15687

@ssciolla ssciolla merged commit 522590e into tl-its-umich-edu:master Mar 3, 2023
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.

Upgrade Postgres in docker-compose from 10 to 14.4
2 participants