Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix doc #2436

Closed
wants to merge 1 commit into from
Closed

Fix doc #2436

wants to merge 1 commit into from

Conversation

jmccl
Copy link
Contributor

@jmccl jmccl commented Feb 24, 2022

Based on where the file is actually put when running through the tutorial.

Based on running through the tutorial
@georglauterbach
Copy link
Member

I'm not quite sure on this, as I did not manage consistency for volume mounts, but the current value seems to be correct to me. It is the location on the host file system within our volume example directories. @polarathene can you confirm?

@georglauterbach georglauterbach added area/documentation kind/bugfix meta/needs triage This issue / PR needs checks and verification from maintainers labels Feb 25, 2022
@polarathene
Copy link
Member

but the current value seems to be correct to me. It is the location on the host file system within our volume example directories.

Current docs are correct, the host volume path is a placeholder but reflects what we use in our example docker-compose volumes:

- ./docker-data/dms/config/:/tmp/docker-mailserver/

@jmcci probably has upgraded an existing install perhaps, before we normalized the volume paths across the docs?


Based on where the file is actually put when running through the tutorial.

@jmccl you say where the file is when running through the tutorial you're updating in the PR? But this volume path is correctly matching the example docker-compose volume path earlier in that tutorial:

- ./docker-data/dms/config/:/tmp/docker-mailserver/

I think the actual correction would be to update this line:

5. [Generate DKIM keys][docs-dkim] for your domain via `./setup.sh config dkim`.

We removed the need for setup.sh as we now include this file with the relevant container image release. Instead the command would be docker exec -it mailserver setup config dkim. This is mentioned early on, but it's probably a good time for our current docs to remove the transition by dropping mention of setup.sh / v10.2?:

1. If you're running a version of `docker-mailserver` earlier than v10.2, [you'll need to get `setup.sh`][docs-setup-script]. Otherwise you can substitute `./setup.sh <command>` with `docker exec mailserver setup <command>`.

Likewise step 8 has some examples with many setup.sh commands that would need to be converted (or shell into the container to runsetup without the docker exec mailserver prefix). That said using the external setup.sh is still a valid alternative AFAIK, just no longer requiring keeping it in sync with the correct image release.

At least it seems that the support for both caused some confusion for this tutorial. setup.sh is setup to use the new path, but still supports the legacy one, so I'm not sure what happened for @jmccl , if they didn't have ./config already created, setup.sh should not have created it...?:

docker-mailserver/setup.sh

Lines 109 to 118 in 2de3340

function _set_default_config_path
{
if [[ -d "${DIR}/config" ]]
then
# legacy path (pre v10.2.0)
DEFAULT_CONFIG_PATH="${DIR}/config"
else
DEFAULT_CONFIG_PATH="${DIR}/docker-data/dms/config"
fi
}

@georglauterbach
Copy link
Member

I see. I think it is a good idea to remove the calls to setup.sh and replace them with the docker exec -ti mailserver setup ... alternative and make the reader aware that setup.sh can do this as well.

Upgrading to DMS >10.2.0 likely caused the confusion here, I agree. The docs should be updated

@casperklein
Copy link
Member

casperklein commented Feb 25, 2022

Another thing I run into myself some time ago. When you are not using the snipped below in an empty directory

DMS_GITHUB_URL='https://raw.githubusercontent.com/docker-mailserver/docker-mailserver/master'
wget "${DMS_GITHUB_URL}/docker-compose.yml"
wget "${DMS_GITHUB_URL}/mailserver.env"
wget "${DMS_GITHUB_URL}/setup.sh"
chmod a+x ./setup.sh

but clone the repo instead, you'll have an config directory. Then setup.sh falls back using the config directory. Newly accounts will then be added under config. When you then start DMS with the default compose file, docker-data/dms/config is used. An error, that at least one email account must exist, will be printed and DMS does not start.

@jmccl
Copy link
Contributor Author

jmccl commented Feb 25, 2022

This was not an upgrade; it was (an attempt at) a clean install. This is my first time trying to use this project.

I started from this page as I generally prefer to start from source rather than rely on build processes I don't understand.

https://github.com/docker-mailserver/docker-mailserver/blob/master/docs/content/examples/tutorials/docker-build.md

I got the source from head yesterday with this command, found on that page.

git clone --recurse-submodules https://github.com/docker-mailserver/docker-mailserver

I then used this page to twiddle the config as the page is about what I'm trying to do.

https://github.com/docker-mailserver/docker-mailserver/blob/master/docs/content/examples/use-cases/forward-only-mailserver-with-ldap-authentication.md

After changing those settings, I went to this page and followed the instructions.

https://github.com/docker-mailserver/docker-mailserver/blob/master/docs/content/examples/tutorials/basic-installation.md

=========================

Things that surprised me:

  • It's not clear to me that you can build from source. It's what I thought I was doing, but setup.sh pulls an image anyway.
  • It wasn't clear to me there were two ways of doing things. I now see the main README is mostly docker-compose based, but most of the rest of the documentation seems to be setup.sh based.

@georglauterbach
Copy link
Member

There really is a discrepancy between how one would now do it vs. the way it was done with setup.sh. This needs some work indeed. However, at no point do we state that setup.sh is used to build DMS.

@jmccl
Copy link
Contributor Author

jmccl commented Feb 25, 2022

I agree, I misunderstood. I was just trying to explain what I did and what I was thinking when I did it.

@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 6219a5d

@polarathene
Copy link
Member

polarathene commented Feb 25, 2022

  • It's not clear to me that you can build from source. It's what I thought I was doing, but setup.sh pulls an image anyway.

Building from source would be our Dockerfile, just as the example doc you referenced detailed.

setup.sh is post-setup, perhaps a different name would be more suited like config.sh, manage.sh or util.sh.. Originally it was required to use this external helper script file, but in the 10.x release series we've since integrated the bulk of the functionality into the image.

The external script just provides some extras like pulling the image if it's not locally available. By default it's set to out :latest release tag:

DEFAULT_IMAGE_NAME='docker.io/mailserver/docker-mailserver:latest'

setup.sh help (also covered in our docs) will also show that you can override that default with your own locally built image, or running container (when appropriate):

-i IMAGE_NAME
Provides the name of the 'docker-mailserver' image. The default value is
'${WHITE}${DEFAULT_IMAGE_NAME}${RESET}'
-c CONTAINER_NAME
Provides the name of the running container.
-p PATH
Provides the local path of the config folder to the temporary container instance.
Does not work if an existing a 'docker-mailserver' container is already running.

It wasn't clear to me there were two ways of doing things. I now see the main README is mostly docker-compose based, but most of the rest of the documentation seems to be setup.sh based.

You can use docker-compose.yml, Docker CLI or other alternatives (Podman / kubernetes). setup.sh <command> or docker exec mailserver setup <command> are the two different approaches for calling the same commands (email, alias, quota, config, relay, debug), the docs thus use setup.sh for that since that was previously the only way and there isn't much difference beyond preference.

For you it was a little jarring, since you strayed from our "happy" path by choosing to begin with your own local build (which is fine, but mostly documented for contributors). You're welcome to improve our docs for that experience if you'd like :)


This was not an upgrade; it was (an attempt at) a clean install. This is my first time trying to use this project.

I started from this page as I generally prefer to start from source rather than rely on build processes I don't understand.

Totally fine 👍

I then used this page to twiddle the config as the page is about what I'm trying to do.

Just a little warning (although I think we've made it more clear recently), LDAP support is fully community contributed, we don't have any maintainers that use it or know much to maintain it well, and users of LDAP have raised some issues that are yet to be resolved if you run into those.

After changing those settings, I went to this page and followed the instructions.

You'd probably want to start off with the basic installation first, then repeat for extras like LDAP I'd have thought? 😅

Still I don't see anything there that would have caused you to end up with the old config volume path that you're contributing here? Were you not using docker-compose? How did you configure your config volume? (EDIT: Resolved, technical mishap caused by cloning our git repo and running setup.sh at the git project root)

@polarathene
Copy link
Member

polarathene commented Feb 25, 2022

but clone the repo instead, you'll have an config directory. Then setup.sh falls back using the config directory.

@casperklein Oops I missed this comment! That explains it then 😅

Perhaps we could fix that? Either by dropping the legacy support from setup.sh in a future release (v11?) or renaming the repo config directory?

@georglauterbach
Copy link
Member

but clone the repo instead, you'll have an config directory. Then setup.sh falls back using the config directory.

@casperklein Oops I missed this comment! That explains it then sweat_smile

Perhaps we could fix that? Either by dropping the legacy support from setup.sh in a future release (v11?) or renaming the repo config directory?

I vote for dropping support of the legacy configuration in v11. If you agree, 👍🏼 , if not, 👎🏼 :D

@casperklein
Copy link
Member

Ideally we should do both. We chose for reasons to rename config to ./docker-data/dms/config in our sample configuration. Consequently we should rename the config directory accordingly, to re-establish the same functionality as before: you could clone this repo and had a bunch of sample configuration in config. This was missed in the first place when the default was changed.

@casperklein
Copy link
Member

PS: I find the new directory structure still weird. docker-data/dms suggest, that docker-data is a general docker directory, with DMS as one of many projects. In reality, I doubt, that someone will ever use this directory to place other docker-data in, beside DMS. Why would anyone want to mix-up his DMS stack with data not related to DMS.
While I understand the reasons for the renaming (easier handling of our docs), personally I find it strange to have the proposed directory structure in this repo.

However, no matter what we do, we should do it consequently. And at the moment, this means to rename config to docker-data/dms/config.

@georglauterbach
Copy link
Member

georglauterbach commented Feb 26, 2022

I agree with @casperklein that the directory structure proposed in our docs is, at least IMHO, not likely to be found out there in the wild (i.e. a bit weird). We could consistently rename it though.

@casperklein casperklein mentioned this pull request Feb 26, 2022
11 tasks
@casperklein
Copy link
Member

I've created a PR. Let's continue to discuss in #2438.

@jmccl
Copy link
Contributor Author

jmccl commented Feb 26, 2022

FWIW, I had to move the 'opendkim' directory from 'config' where it was placed to 'docker-data/dms/config' in order for the mailserver to recognize the dkim configuration, So I agree that the pr is the right thing, while leaving the doc as is. As such, I'll close this pr.

@jmccl jmccl closed this Feb 26, 2022
@jmccl
Copy link
Contributor Author

jmccl commented Feb 26, 2022

BTW, thanks to all of you for looking at this so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation meta/needs triage This issue / PR needs checks and verification from maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants