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

Update Docker-compose file #1071

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Ni-g-3l
Copy link
Contributor

@Ni-g-3l Ni-g-3l commented Aug 6, 2023

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).

  • Update postgres alpine version
  • Update README and Docker Docs
  • Update docker-compose file

Copy link
Collaborator

@mockdeep mockdeep left a comment

Choose a reason for hiding this comment

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

@guidopetri do you have time to look this over?

- POSTGRES_USER=db_user
- POSTGRES_DB=stringer

web:
image: mockdeep/stringer
image: mockdeep/stringer:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come you chose version 12 instead of the most recent 15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just upgraded my instance, let's pass this week and see what will happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First issues incomming, you can't point to same folder postgres12 and postgres15
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this change to me? I'm not at all familiar with docker, so not sure why this would change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Existing users will need to move their directory from /srv/stringer/data to ./postgres, yeah. I think this is a better default, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines 28 to 31
- 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

build: .
depends_on:
- postgres
restart: always
ports:
- 80:8080
- '127.0.0.1:8080:8080'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@Ni-g-3l
Copy link
Contributor Author

Ni-g-3l commented Aug 12, 2023

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds reasonable. I think the network updates might be a good 4th PR, too.

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