Update all docker-compose files to use the same version and examples by williamdes · Pull Request #2159 · docker-mailserver/docker-mailserver

williamdes · 2021-08-29T19:31:32Z

Description

Update all docker-compose files to use the same version and examples

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

wernerfred

All in all thanks for syncing up all those examples.
But i really dislike inline comments (no benefit except for extra work when automating and scripting stuff)
So if we really need those comments at all then please put them in the line above. As most of the occurences are examples imho it is fine to keep mail and example.com . I think we can expect this level of knowledge transfer capacity of people that want to run their own mailserver.

williamdes · 2021-08-29T20:08:19Z

So if we really need those comments at all then please put them in the line above. As most of the occurences are examples imho it is fine to keep mail and example.com . I think we can expect this level of knowledge transfer capacity of people that want to run their own mailserver.

Well, let me know what example if best so I can sync all of them
I would like to let some of the comments as users are not always aware values need to be changed even if it is obvious for us having the knowledge

casperklein · 2021-08-29T20:10:25Z

I think this is fine (current README):

version: '3.8'

services:
  mailserver:
    image: docker.io/mailserver/docker-mailserver:latest
    hostname: mail
    domainname: example.com
    container_name: mailserver

georglauterbach

Two small changes I proposed. Other than that, LGTM 👍🏼

williamdes · 2021-08-29T21:25:00Z

+                - ONE_DIR=1
+                - DMS_DEBUG=0
+                - SPOOF_PROTECTION=0
+                - REPORT_RECIPIENT=1


By the way, this is deprecated, what should it be changed to ?
The docs are not clear about that

What exactly is deprecated? ONE_DIR ?

https://docker-mailserver.github.io/docker-mailserver/v10.0/config/environment/#report_recipient-deprecated

REPORT_RECIPIENT :)

https://docker-mailserver.github.io/docker-mailserver/v10.0/config/environment/#pflogsumm_recipient

PFLOGSUMM_RECIPIENT or leave it blank

The space count seems wrong (4 instead of 2) for the ports, volumes and environment items.

I have already addressed indent issues in my commits. This file in particular gets it's own separate PR due to it's history and issues with it in general I've resolved, that will follow after my bulk changes PR.

casperklein · 2021-08-29T21:34:11Z

I've aligned the volumes in docs/content/examples/tutorials/basic-installation.md with our default docker-compose.yml.

wernerfred · 2021-08-30T06:19:27Z

+                - ONE_DIR=1
+                - DMS_DEBUG=0
+                - SPOOF_PROTECTION=0
+                - REPORT_RECIPIENT=1


https://docker-mailserver.github.io/docker-mailserver/v10.0/config/environment/#pflogsumm_recipient

PFLOGSUMM_RECIPIENT or leave it blank

polarathene

This looks great @williamdes ! Thanks for addressing this 😀

An observation that I couldn't easily add a review comment on, was some inconsistencies in volumes. The main concern with that being older data volumes mapping vs newer bind mount volumes that we changed to by default this year.

Any of these mappings should take precedence over the previous data volumes usage I think?:

    volumes:
      - ./data/maildata:/var/mail
      - ./data/mailstate:/var/mail-state
      - ./data/maillogs:/var/log/mail
      - /etc/localtime:/etc/localtime:ro
      - ./config/:/tmp/docker-mailserver/

May be easiest to search based on the the internal mapped location, there is bind mount examples using ./data/ prefix followed by mail,state,logs` as a variant too. Perhaps we should make that consistent?

./data/mail* seems redundant vs ./data/* for the prefix, alternatively unify the config volume under the same hierarchy, ie: ./data/config/.

Since some compose examples have other services doing local mounts, might want to group those volumes to the context of the service (dms):

./dms/config/ and ./dms/data/* (mail, state, logs).
or ./data/dms/config/ and ./data/dms/*.

Some examples only mount some of those volumes, I assume because the others aren't required for the example.

In the ssl.md doc, nginx related services are using absolute volume paths like /mnt/data/nginx, should probably match relative path as well (except for /var/run/docker.sock). The related dms container just has ./mail and ./mail-state and mixes in the absolute nginx path to share the cert.

On basic-installation.md, I believe @wernerfred might have created this doc from splitting a wiki article of his during the migration to new docs. It needs to be reviewed from the looks of it as it includes the volume mount: /var/ds/wsproxy/letsencrypt/:/etc/letsencrypt/, this is described further down on the page, but the referenced docker image isn't included in the compose example above, I don't think the absolute path is required? If updating this keep in mind the content on the document outside the config snippet referencing it.

As the example docker-compose.yml at the root of the git repo has comments to provide guidance on ports, should there also be one above hostname to clarify that the two properties define the FQDN mail.example.com? I think when I first encountered that (not something I was used to configuring before docker-mailserver), I was initially confused with what to expect.

polarathene · 2021-09-03T04:23:05Z

      containers:
        - name: mailserver
-          image: ghcr.io/docker-mailserver/docker-mailserver:10.0.0
+          image: ghcr.io/docker-mailserver/docker-mailserver:latest


Suggested change

image: ghcr.io/docker-mailserver/docker-mailserver:latest

image: docker.io/docker-mailserver/docker-mailserver:latest

Why not change this to docker.io? (this was marked as resolved)

I have done so on my additions. I marked this as resolved when I committed it.

Originally I was going to push here, but other changes piled up. If you would like I can commit the suggestion here and rebase my changes on top?

williamdes · 2021-09-07T12:08:33Z

Hi,
I am too much overbooked now, could other maintainers apply the minor changes that are needed ? 🙏🏻

polarathene · 2021-09-08T02:31:01Z

I am too much overbooked now

No immediate rush for this AFAIK.

Thanks for letting us know, if someone has the time to spare we'll leave a comment upon picking it up :)

polarathene · 2021-09-18T00:22:28Z

I am now wrapping up this PR.

polarathene · 2021-09-20T00:51:50Z

My additions are too large to mix into this PR. I have some questions aimed at my changes and due to the noise it would add, I think it's better addressed via separate PR than to dirty the review state that reached approval here.

Unfortunately it seems there's some conflicts already, I'll address those and we can approve/merge?

github-actions · 2021-09-20T01:06:03Z

Documentation preview for this PR is ready! 🎉

Built with commit: 1b1c07a

polarathene

Resolved the conflicts. setup.sh.md and mailserver-behind-proxy.md updated to main branch.

williamdes commented Aug 29, 2021

View reviewed changes

Comment thread docs/content/examples/tutorials/mailserver-behind-proxy.md

williamdes commented Aug 29, 2021

View reviewed changes

Comment thread docs/content/examples/tutorials/basic-installation.md

williamdes commented Aug 29, 2021

View reviewed changes

Comment thread docs/content/examples/tutorials/mailserver-behind-proxy.md Outdated

williamdes requested review from georglauterbach and wernerfred August 29, 2021 19:32

wernerfred reviewed Aug 29, 2021

View reviewed changes

casperklein added area/documentation kind/improvement Improve an existing feature, configuration file or the documentation priority/low labels Aug 29, 2021

casperklein assigned williamdes Aug 29, 2021

casperklein modified the milestones: v10.1.2, v10.2.0 Aug 29, 2021

georglauterbach requested changes Aug 29, 2021

View reviewed changes

Comment thread README.md

Comment thread docs/content/config/security/ssl.md Outdated

williamdes added 2 commits August 29, 2021 23:11

Set DMS_DEBUG to 0

2d7c4ed

Update setup example output

7dbca05

casperklein previously approved these changes Aug 29, 2021

View reviewed changes

Bump to traefik:v2.5

31c6597

williamdes dismissed casperklein’s stale review via 31c6597 August 29, 2021 21:18

This comment has been minimized.

Sign in to view

casperklein previously approved these changes Aug 29, 2021

View reviewed changes

williamdes commented Aug 29, 2021

View reviewed changes

Comment thread docs/content/examples/tutorials/basic-installation.md

williamdes requested a review from georglauterbach August 29, 2021 21:21

williamdes commented Aug 29, 2021

View reviewed changes

align with default docker-compose.yml

fc3d5e1

casperklein dismissed their stale review via fc3d5e1 August 29, 2021 21:31

casperklein previously approved these changes Aug 29, 2021

View reviewed changes

wernerfred reviewed Aug 30, 2021

View reviewed changes

update docs to not use my VM test user anymore

027e2b1

georglauterbach dismissed casperklein’s stale review via 027e2b1 August 30, 2021 08:27

fixed my own typo...

0b4b042

georglauterbach previously approved these changes Aug 30, 2021

View reviewed changes

wernerfred reviewed Aug 30, 2021

View reviewed changes

Comment thread docs/content/config/setup.sh.md Outdated

polarathene requested changes Sep 3, 2021

View reviewed changes

casperklein marked this pull request as draft September 8, 2021 10:52

georglauterbach modified the milestones: v10.2.0, v10.3.0 Sep 13, 2021

polarathene self-assigned this Sep 18, 2021

polarathene marked this pull request as ready for review September 20, 2021 00:46

polarathene previously approved these changes Sep 20, 2021

View reviewed changes

Merge branch 'master' into docs-fix

1b1c07a

polarathene dismissed stale reviews from georglauterbach and themself via 1b1c07a September 20, 2021 01:05

polarathene approved these changes Sep 20, 2021

View reviewed changes

georglauterbach approved these changes Sep 20, 2021

View reviewed changes

wernerfred approved these changes Sep 20, 2021

View reviewed changes

polarathene merged commit 4d3fade into docker-mailserver:master Sep 20, 2021

polarathene mentioned this pull request Sep 22, 2021

docs(chore): Normalize for consistency #2206

Merged

7 tasks

williamdes deleted the docs-fix branch September 22, 2021 18:13

	image: ghcr.io/docker-mailserver/docker-mailserver:latest
	image: docker.io/docker-mailserver/docker-mailserver:latest

Uh oh!

Conversation

williamdes commented Aug 29, 2021

Description

Type of change

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wernerfred left a comment

Choose a reason for hiding this comment

Uh oh!

williamdes commented Aug 29, 2021

Uh oh!

casperklein commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein commented Aug 29, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

williamdes commented Sep 7, 2021

Uh oh!

polarathene commented Sep 8, 2021

Uh oh!

polarathene commented Sep 18, 2021

Uh oh!

polarathene commented Sep 20, 2021

Uh oh!

github-actions Bot commented Sep 20, 2021

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

casperklein commented Aug 29, 2021 •

edited

Loading

casperklein Aug 29, 2021 •

edited

Loading

casperklein Sep 20, 2021 •

edited

Loading