-
-
Notifications
You must be signed in to change notification settings - Fork 33
Add installation using Docker Compose and many other improvements for implementation in Docker. #614
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
base: main
Are you sure you want to change the base?
Conversation
- Add markdown tabs blocks - Fix [Issue 604](chatmail#604) - Add `--skip-dns-check` argument to `cmdeploy run` command - Add `--force` argument to `cmdeploy init` command - Add startup for `fcgiwrap.service` - Add extended check when installing `unbound.service` - Add configuration parameters (`is_development_instance`, `use_foreign_cert_manager`, `acme_email`, `change_kernel_settings`, `fs_inotify_max_user_instances_and_watchers`)
great work and offer of contribution, @Keonik1 -- the silence since friday is more accidental but several people are excited about this PR and want to try it out and feedback. Not doing that myself right now but wanted to let you know :) |
I am very interested in deploying with containers as well. In particular, I would like to decouple from systemd and the host operating system, so that the software content of the components can be managed independently. Doing that will require an understanding of what all the components are, and toward that end, I created #615 to document what I know so far. I like the re-use here of the pyinfra-based script to populate the container; I think sharing the process as much as possible between containers and host-based deploys is a good idea. I think with some revisions the process could use pyinfra's |
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.
Soo many changes :) thanks for all the effort and thoughts that went into this.
Sorry for taking some days to reply, but I wanted to look at it in full detail, and have some fix suggestions already. I got acmetool to work, for example.
The tests are mostly fine with this PR, but unfortunately many tests try to login via SSH - maybe we can try to run docker exec
if a chatmail container is running on the same machine?
A github action which deploys the docker container automatically would also be nice :) We can use a hetzner VPS for that, like for the cmdeploy CI. It would be good to have CI notice when something fails with a new change, to avoid regressions.
In general, not bad how many variables and configuration options you added, e.g. that CHANGE_KERNEL_SETTINGS
is set to False by default in setup_chatmail_docker
. In most situations the defaults will probably never be overridden, but it's good that it's explicit what's the behavior.
The markdown tabs are also a nice touch, but we might need more discussion to merge them into main. The question is, which languages do you prefer, which do you offer? Only offering english is not much better, sure. It requires some thought, I think, or could go to an example page so admins can choose to have the tabs if they want to be multi-lingual. Right now, many admins just translate the www directory to their mother tongue.
All in all, thanks for all of this! I suggest to address the comments, and merge it to a docker
branch for now, so we can make it more stable in subsequent PRs. But in any case there will be some interested testers already :)))
cmdeploy/src/cmdeploy/cmdeploy.py
Outdated
def get_sshexec(): | ||
print(f"[ssh] login to {args.config.mail_domain}") | ||
return SSHExec(args.config.mail_domain, verbose=args.verbose) | ||
host = args.ssh_host if hasattr(args, "ssh_host") and args.ssh_host else args.config.mail_domain | ||
print(f"[ssh] login to {host}") | ||
return SSHExec(host, verbose=args.verbose) |
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.
This is a good step towards getting cmdeploy dns
to run as well... but maybe we can completely avoid logging in to localhost? That would be much simpler. Let's see, for now at least deploying works, after all :)
out.green(f"created config file for {mail_domain} in {args.inipath}") | ||
if not args.recreate_ini: | ||
out.green(f"[WARNING] Path exists, not modifying: {inipath}") | ||
return 0 |
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.
This makes cmdeploy/src/cmdeploy/tests/test_cmdeploy.py::TestCmdline::test_init_not_overwrite
fail - let's adjust that test so it also tests the overwrite variable?
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.
I don't know how to do this correctly :(
Can you attach an suggesstion?
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.
Neural network wrote this but I can't check how this works, may be you can?
def test_init_creates_file(self, capsys):
assert main(["init", "chat.example.org"]) == 0
out, err = capsys.readouterr()
assert "created config file for" in out.lower()
def test_init_not_overwrite(self, capsys):
assert main(["init", "chat.example.org"]) == 0
capsys.readouterr()
assert main(["init", "chat.example.org"]) == 0
out, err = capsys.readouterr()
assert "[warning] path exists" in out.lower()
def test_init_force_recreates_file(self, capsys):
assert main(["init", "chat.example.org"]) == 0
capsys.readouterr()
assert main(["init", "chat.example.org", "--recreate-ini"]) == 0
out, err = capsys.readouterr()
assert "[warning] force argument was provided" in out.lower()
assert "created config file for" in out.lower()
@missytake, Hello! I've also thought about it and will try to send fixes in the format of |
i have 2 ideas:
It's also probably worth adding the ability to determine which languages will be displayed in the installed instance, so that admins can simply enable or disable if necessary. I'll think about the implementation of this at the end, when I fix everything else. |
Hi, when I decided to set up a chatmail server for personal use, I was frustrated to discover that there was no Docker installation, so I created a preliminary implementation.
Brief description of changes
--skip-dns-check
argument tocmdeploy run
command--force
argument tocmdeploy init
commandfcgiwrap.service
unbound.service
is_development_instance
,use_foreign_cert_manager
,acme_email
,change_kernel_settings
,fs_inotify_max_user_instances_and_watchers
)Full description of changes
The main thing that was done was to add support for running in Docker, but in addition to that, the problems that made this method of running impossible were also solved.
I think the advantages of running in Docker are obvious:
This is only the first implementation of installation using Docker, but it works and seems to be stable (at least I haven't found any anomalies), but in the future it will need to be brought to a more adequate state (with the abandonment of using systemd inside and the division of everything into several containers or 1 container, but without systemd management, but that's just a thought for the future).
The modifications were made with a focus on full compatibility with the current installation and are additional optional parameters.
In addition, some QoL features and instance documentation unification were added so that administrators would not have to create separate pages for different language groups of users.
Some errors that I encountered during deployment have also been fixed (and to be honest, I don't understand why some of them didn't occur for others -_-), namely:
--ssh_host
argumentfcgiwrap.service
service, which does not download or start when rolled out using the current methodunbound.service
, when it is installed but does not show that it occupies port 53All changes are described in detail in the
changelog.md
.I hope my improvements meet your requirements and you accept the PR, because I really believe that this is a very important part of the project's development — installation using Docker is easier, which means that many more people will be able to install it and thus popularize it.