-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
&& apt-get clean \ | ||
&& rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | ||
|
||
COPY docker/rootfs/ / |
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.
Since L17 does a copy of ./, then AFAICT, you can replace this line by a "cp -r /synapse/source/docker/rootfs/* /" added to the RUN statement above (somewhere before the rm-rf /synapse/source), to avoid another layer in the image.
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.
True.
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.
Though we're discussing whether to have this Dockerfile in-tree or out-of-tree. In-tree means it's easier to build an image from dev code. Out-of-tree feels a bit cleaner and it can be maintained separate from synapse releases.
|
||
## Run | ||
|
||
**NOTE**: If you are not using postgresql and are using sqlite3 as your database, you will need to make a directory to store the sqlite3 database file in and then mount this volume into the container at `/synapse/data/`. As it is so easy to use postgresql, when using Docker containers, this is not documented to somewhat discourage it. Choose a `POSTGRES_PASSWORD` instead. |
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.
Isn't the data directory still needed even with postgresql, for the content repository? (In fact, I don't see where media_store_path gets set when the config gets generated.)
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.
Good points. That needs fixing.
Is there a plan to also run matrix.org from the same docker images? Or docker images built from the same Dockerfile? |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
FROM phusion/baseimage:0.9.22 |
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.
May I ask why this is used as a base image? As there is only one process running in this image (Synapse), afaik there is no advantage in using the extra supervisor employed by phusion-baseimage. The official ubuntu
( or debian
) base image will probably be enough.
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.
+1. The ubuntu base image is better maintained, smaller in size, and in 10X broader usage. It seems most of the things phusion is solving for are no longer issues.
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.
If an additonal opinion would be asked here, we're also running from debian:jessie
in https://github.com/allmende/docker-synapse/blob/master/Dockerfile#L1 without issues.
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.
alpine can also be used as the base, see https://github.com/ptman/synapse-docker
This probably needs to be a fairly high priority seeing as how there is an "official" build on dockerhub that isn't being maintained (also must pull v0.22.1, :latest doesn't work). |
Hi Rob, we're probably going to go with adopting the docker image provided by kaiyou (#2846) rather than this image, so I'm going to close this PR now. |
No description provided.