search

AVENTER-UG / docker-matrix

docker image for matrix.org
https://riot.im/app/#/room/#dockermatrix:matrix.aventer.biz
GNU General Public License v2.0
93 stars 19 forks source link

use su matrix -c to start #29

Closed mvgorcum closed 5 years ago

mvgorcum commented 5 years ago

Because the previous PR broke backwards compatibility. This should fix that.

mvgorcum commented 5 years ago

This is quite an ugly hack, and much like the original container, the container itself still runs as root, even though synapse does run as the matrix user.

The merged python 3 PR (#28) uses the USER command, which may have some issues with people that changed the uid/gid. If that is the case, chown-ing the mapped volume folder to 991 should fix the issue, or the container can be run with --user.

All in all: while #28 breaks backwards compatibility for some (probably not many) users, I would advocate using #28 rather than this one.

evoL commented 5 years ago

I respectfully disagree with the idea that #28 is a better way to go. The PR essentially removed the feature to configure the MATRIX_UID/MATRIX_GID environment variables, which is a regression from the previous state. As a user, I wasn't prepared for this.

I strongly believe that breaking backwards compatibility should be only used as a last resort. If this PR makes the Python 3 images compatible again, I'd be in favor of merging it in.

If you still want to go with the approach of #28, please at least change the README to remove all mentions that the UID/GID is configurable and add a short "Breaking changes" section that describes the matter and outlines a migration path.

andreaspeters commented 5 years ago

Like we already discuss in the matrix chat, I will not merge this PR. The bug is fixed without to lose the UID feature. :-) Thanks a lot @mvgorcum.