Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Improvements for Docker usage #3543

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Improvements for Docker usage #3543

merged 2 commits into from
Aug 1, 2018

Conversation

bebehei
Copy link
Contributor

@bebehei bebehei commented Jul 17, 2018

Small improvement for the dockerfile to match best practices and a small improvement to make the low level docker call unnecessary.

It's much easier to build the image via docker-compose instead of an
error-prone low-level docker call.

Signed-off-by: Benedikt Heine <[email protected]>
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

1 similar comment
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@richvdh richvdh requested a review from a team July 19, 2018 09:13
Copy link
Contributor

@jcgruenhage jcgruenhage left a comment

Choose a reason for hiding this comment

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

LGTM

```

The `-t` option sets the image tag. Official images are tagged `matrixdotorg/synapse:<version>` where `<version>` is the same as the release tag in the synapse git repository.
Build the docker image with the `docker-compose build` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should document that people can skip building and specify the version where we have latest in the docker-compose file, to pull it from docker hub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, full ACK. But my intention is to fix this later in a separate PR. This is my first one and I wanted to get the PR process known first.

This matches docker best practices.

Signed-off-by: Benedikt Heine <[email protected]>
@bebehei
Copy link
Contributor Author

bebehei commented Jul 31, 2018

Hey guys, what's missing in this PR to get finally merged?

@jcgruenhage
Copy link
Contributor

Nothing as far as I can tell. @richvdh, was I supposed to merge this after approving it? I guess I did something wrong and this fell off the radar because of that.

@richvdh
Copy link
Member

richvdh commented Aug 1, 2018

Nothing as far as I can tell. @richvdh, was I supposed to merge this after approving it?

yes, if you're happy, please merge it :)

@jcgruenhage jcgruenhage merged commit c4842e1 into matrix-org:develop Aug 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants