Update Docker-compose file #1071
Conversation
| - POSTGRES_USER=db_user | ||
| - POSTGRES_DB=stringer | ||
|
|
||
| web: | ||
| image: mockdeep/stringer | ||
| image: mockdeep/stringer:latest |
There was a problem hiding this comment.
these images should actually move to stringerrss/stringer at some point. Not sure if now is the time, since we're currently unable to publish the image just yet.
| -e POSTGRES_DB=stringer \ | ||
| postgres:9.5-alpine | ||
| postgres:12-alpine |
There was a problem hiding this comment.
How come you chose version 12 instead of the most recent 15?
There was a problem hiding this comment.
I had an issues with postgres 9 which update all my folder permissions. Apparently it was fixed in 12 version, and it's the first version with the fix. May be you can use 15 version, but I haven't try it.
I could do some test if you want !
There was a problem hiding this comment.
@Ni-g-3l sure, that would be helpful! I'm running 15.3 with my instance. It's not a huge deal, though, we can always upgrade more later.
There was a problem hiding this comment.
I just upgraded my instance, let's pass this week and see what will happen.
There was a problem hiding this comment.
First issues incomming, you can't point to same folder postgres12 and postgres15
There was a problem hiding this comment.
I see. I wonder if upgrading to 12 will cause issues for upgrading users, too. I guess we can leave it at 12 for now and see how things go.
There was a problem hiding this comment.
I think this is more because of an upgrade of postgres using the same data directory, rather than a problem with postgres 15 itself. So updating from 9 to 12 is already going to give issues, I think we may as well do 15.
I think this is probably where a stringer export comes in - we could export the data, upgrade to a newer version of postgres, and then re-import.
imo, we should set this to 15. If a new user comes to stringer, they'll want the latest version of postgres; if a previous user is updating, they should know that they need to back up their system and that changing postgres versions incurs problems. We may want to announce this in some way in the next release though, as a PSA.
There was a problem hiding this comment.
(also, fwiw, someone else I know from the FOSS community created this repo that auto-upgrades postgres without issues like this, and then runs the container as postgres normally would. But it's an additional layer of complexity, and I imagine postgres will eventually coalesce things into their docker image)
There was a problem hiding this comment.
Hmm, I'm not sure what the best approach here is. I'm imagining the upgrade process would be smoother if it was more gradual, but that seems like a pretty lengthy process. That repo you linked could be really handy to smooth that process.
We may want to announce this in some way in the next release though, as a PSA.
I unfortunately haven't figured out a good release process yet. Maybe we need to set up a changelog and tagging somehow. It's a little weird, though, since we aren't releasing a gem or the like.
docs/Docker.md
Outdated
| @@ -20,11 +20,12 @@ The following steps can be used to setup Stringer on Docker, using a Postgres da | |||
| docker run --detach \ | |||
| --name stringer-postgres \ | |||
| --restart always \ | |||
| --volume /srv/stringer/data:/var/lib/postgresql/data \ | |||
| --volume ./postgres:/var/lib/postgresql/data \ | |||
There was a problem hiding this comment.
Can you explain this change to me? I'm not at all familiar with docker, so not sure why this would change.
There was a problem hiding this comment.
I thought it would be better to gives to people a command which isn't install things all around your disks. Like that a postgres folder will be created to handle the docker volume at the current working directory.
I can revert this part if you want !
There was a problem hiding this comment.
How will this impact users who have existing installations? Will they need to move their data directory? I think ideally we wouldn't make upgrades more trouble, at least until other upgrade issues are resolved.
There was a problem hiding this comment.
Existing users will need to move their directory from /srv/stringer/data to ./postgres, yeah. I think this is a better default, though.
There was a problem hiding this comment.
I noticed in the docker-compose that the default dir there is ~/stringer which imo is an even better default.
| environment: | ||
| - POSTGRES_PASSWORD=super_secret_password | ||
| - POSTGRES_PASSWORD=<PLEASE_ENTER_YOUR_PASSWORD> |
There was a problem hiding this comment.
Personally I don't care too much for changes like these since the password will need to be changed anyway. If we want to make it so that the container won't launch without a password change then we should probably just not set this value
There was a problem hiding this comment.
I'm fine with cosmetic changes, and this seems like maybe an improvement. If there's a way to force the user to input it, though, so that they can't miss it, that seems even better.
docker-compose.yml
Outdated
| - SECRET_KEY_BASE=$(openssl rand -hex 64) | ||
| - ENCRYPTION_PRIMARY_KEY=$(openssl rand -hex 64) | ||
| - ENCRYPTION_DETERMINISTIC_KEY=$(openssl rand -hex 64) | ||
| - ENCRYPTION_KEY_DERIVATION_SALT=$(openssl rand -hex 64) |
There was a problem hiding this comment.
I'm not a huge fan of this - this will generate new keys every time. So if someone accidentally re-ups the container, I think it ends up generating new keys and so their data is now locked behind (lost) encryption keys.
docker-compose.yml
Outdated
| build: . | ||
| depends_on: | ||
| - postgres | ||
| restart: always | ||
| ports: | ||
| - 80:8080 | ||
| - '127.0.0.1:8080:8080' |
There was a problem hiding this comment.
This will only work for users who have a reverse proxy on their machine; it's safer this way (and imo should be the way to go) but it's something to be aware of when merging this in. The docker compose file doesn't actually have a reverse proxy listed in it.
|
Hey ! I followed up your recommendation and I made changes. Feel free to comment If I missed something |
| - DATABASE_URL=postgres://db_user:super_secret_password@postgres:5432/stringer | ||
| - DATABASE_URL=postgres://db_user:<PLEASE_ENTER_YOUR_PASSWORD>@stringer-postgres:5432/stringer | ||
|
|
||
| networks: |
There was a problem hiding this comment.
Do you think you could separate out some of these changes into different PRs? I think it would be helpful to discuss them independently so that I can get a better understanding of what each of them is doing. I'm sure some of them would be easy to merge, but others seem like they might cause more problems for our users and I'd like to think about them more.
There was a problem hiding this comment.
Hey no problem, I can separate into :
- Pg upgrade and location
- Cosmetic Improve (README, etc...)
- Docker Image source
If you validate this I will do it during the week !
There was a problem hiding this comment.
That sounds reasonable. I think the network updates might be a good 4th PR, too.
Hey !
I was in lot of trouble on deploying the stringer docker-compose to my Portainer. So this is a little PR to fix it.
Feel free to report any mistakes in the PR (I'm at the beginning of my Open Source adventure).