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

Refactor Dockerfile location #3644

Merged
merged 8 commits into from
Aug 3, 2018
Merged

Refactor Dockerfile location #3644

merged 8 commits into from
Aug 3, 2018

Conversation

michaelkaye
Copy link
Contributor

This is based on the changes in #3253 but old branch is old, so tidying it up and re-submitting as a new, clean PR.

This is step one in a three stage plan:

  1. Move docker file around so README from the root of synapse doesn't trample the readme on the docker-hub all the time. We want different information in each place so sharing the same README doesn't seem sensible.
  2. Enable automated builds of tags, and develop (only possible as each time you build on docker-hub the readme is updated, so these automated builds trample the README each time, which we re-upload a nice copy of after the manual trigger is complete).
  3. Enable automated builds of master to the ":latest" tag which enables people to shoot themselves in the foot by ignoring versioning of their synapse installation and configuration (opinions differ on whether that's a good idea)

@michaelkaye michaelkaye requested review from jcgruenhage and a team August 2, 2018 17:38
richvdh
richvdh approved these changes Aug 2, 2018
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks entirely plausible to me, but my docker-fu is negligible.

contrib/docker/README.md Outdated
* ``SYNAPSE_SMTP_PORT``, TCP port for accessing the mail server [default ``25``].
* ``SYNAPSE_SMTP_USER``, username for authenticating against the mail server if any.
* ``SYNAPSE_SMTP_PASSWORD``, password for authenticating against the mail server if any.
For more information on required environment variables and mounts, see the main docker documentation at `docker/README.md`
Copy link
Member

Choose a reason for hiding this comment

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

can has link?

README.rst Outdated
@@ -157,7 +157,7 @@ if you prefer.

In case of problems, please see the _`Troubleshooting` section below.

There is an offical synapse image available at https://hub.docker.com/r/matrixdotorg/synapse/tags/ which can be used with the docker-compose file available at `contrib/docker`. Further information on this including configuration options is available in `contrib/docker/README.md`.
There is an offical synapse image available at https://hub.docker.com/r/matrixdotorg/synapse/tags/ which can be used with the docker-compose file available at `contrib/docker`. Further information on this including configuration options is available in the README on hub.docker.com.
Copy link
Member

Choose a reason for hiding this comment

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

can has link?

README.rst Outdated
@@ -157,7 +157,7 @@ if you prefer.

In case of problems, please see the _`Troubleshooting` section below.

There is an offical synapse image available at https://hub.docker.com/r/matrixdotorg/synapse/tags/ which can be used with the docker-compose file available at `contrib/docker`. Further information on this including configuration options is available in `contrib/docker/README.md`.
There is an offical synapse image available at https://hub.docker.com/r/matrixdotorg/synapse/tags/ which can be used with the docker-compose file available at `contrib/docker`. Further information on this including configuration options is available in the README on hub.docker.com.
Copy link
Member

Choose a reason for hiding this comment

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

also, can has wrapping at 80 chars?

docker/README.md Outdated
file or with a custom configuration that requires manual editing.

An easy way to make use of this image is via docker-compose. See the
(https://github.com/matrix-org/synapse/tree/develop/contrib/docker)[contrib]
Copy link
Member

Choose a reason for hiding this comment

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

that's not how you link in MD. think it's [text](http://link).

@michaelkaye
Update README.rst
feacd13 
wrap at 80ish
@michaelkaye
Update README.rst
637b11b 
Link to contrib/docker
@michaelkaye
Update README.md
c3f5961 
Link to docker/README.md
@michaelkaye
Update README.md
42960aa 
Link to contrib/docker
jcgruenhage
jcgruenhage approved these changes Aug 3, 2018
View reviewed changes
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.

I don't know enough about hub.docker.com to know whether it's CI can build something like this, but I know it's possible to build this. LGTM

contrib/docker/README.md
* ``SYNAPSE_SMTP_PORT``, TCP port for accessing the mail server [default ``25``].
* ``SYNAPSE_SMTP_USER``, username for authenticating against the mail server if any.
* ``SYNAPSE_SMTP_PASSWORD``, password for authenticating against the mail server if any.
For more information on required environment variables and mounts, see the main docker documentation at [/docker/README.md](../../docker/README.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is kept for not breaking links I guess?

@michaelkaye
Copy link
Contributor Author

@matrixbot retest this please

@michaelkaye michaelkaye merged commit df7f987 into develop Aug 3, 2018
@turt2live turt2live mentioned this pull request Aug 9, 2018
Closed
@richvdh richvdh mentioned this pull request Sep 18, 2018
Closed
@hawkowl hawkowl deleted the michaelkaye/refactor_docker_locations_v2 branch September 20, 2018 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jcgruenhage jcgruenhage jcgruenhage approved these changes

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelkaye @richvdh @jcgruenhage