-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
scripts: improve custom user-supplied Postfix configuration #2598
Conversation
no need to set it in `setup-stack.sh`
The new way of providing your custom Postfix configuration is similar to the old way, but now, the whole file is appended at once, and then "filtered" so Postfix is okay with it. Users can now override the default Postfix configuration and do not have to worry about providing the config line-by-line. The documentation was adjusted accordingly. It emphasises how the configuration is applied to enable users to understand what they can do and how they can achieve that. Note: The new way of adjusting Postfix _should in theory_ be backwards compatible. (Nothing is ever certain with Postfix...)
I figured `/tmp` is nicer for storing a temprary file :D
This PR is expected to fail until #2597 is merged (because of |
Is there a specific reason we cannot use the same idea for master.cf? |
I guess I was not looking at the manpage of |
Yes. I looked into that too when I did my earlier attempt. We process that differently.. Rather than appending config, it's using a different syntax to update which is intended for single line updates:
This is from our tests. It updates an existing parameter for a service with that
This update format is described as:
That may not cover all desired changes/updates though (there are other modes for
Better to leave as-is. It doesn't seem like Oh, we can do |
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.
Nice improvements to the docs! 😀
The config update logic LGTM! 👍
The other test failure I understand will be resolved by related PR dependency, but I didn't expect this one:
I will re-run the tests to double-check that the changes in this PR are actually causing that. |
The Traefik test is still failing :O |
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
I've looked into this and identified the problem :) The tests are not matching the expected values, which are in fact there but This occurs for quite a few settings with long values, and may be at risk of scripts that use Likewise, if any scripts rely on toggling a setting or similar logic that checks for Fix for now would be to not fold values for storage. But this does not rule out risk with scripts built with |
I've noticed with this change that the logs get a bit of spam with cat /tmp/docker-mailserver/postfix-main.cf >>/etc/postfix/main.cf && \
postconf -n >/tmp/postfix-main-new.cf You'll get logs / output such as the following (blank lines added to show grouping):
I'm not entirely familiar with what's going on, but I guess the This is running in a local VM with an XFS filesystem (virtual disk file on host internal SATA SSD) on a host OS also with XFS that is booted from a raw disk image (originally used for a VM) which is stored on XFS filesystem for an external USB SSD. I don't know if something weird is going on I/O wise there with my setup if you're unable to reproduce. I used
Issue above aside, I assume we'd want to silence those warnings being emitted regardless? |
Very interesting observation @polarathene. I am not sure what the cause is, but I can add a small delay and silence the warnings, as they are indeed not in any way useful :) |
1ca06e2
Breaks existing scripts that expect a settings value to be a single line
Whoops that commit was meant to be a Hope you don't mind, it should fix the failing tests. Which perhaps should be using |
All is well :)
Yes, that may really be a good idea :) But still, we have this error: not ok 256 checking system: /var/log/mail/mail.log is error free in 803ms
# (from function `assert_failure' in file test/test_helper/bats-assert/src/assert.bash, line 140,
# in test file test/tests.bats, line 514)
# `assert_failure' failed
#
# -- command succeeded, but it was expected to fail --
# output (3 lines):
# May 30 13:06:59 mail postfix/master[1432]: /etc/postfix/master.cf: line 71: using backwards-compatible default setting chroot=y
# May 30 13:07:12 mail postfix/master[2485]: /etc/postfix/master.cf: line 71: using backwards-compatible default setting chroot=y
# May 30 13:07:20 mail postfix/master[3549]: /etc/postfix/master.cf: line 71: using backwards-compatible default setting chroot=y
# --
# This is still expected, right? |
Yep, that's due to depending on the other PR. |
Creating a two line $ echo -e "max_idle = 300s\nmax_idle = 600s" >/etc/postfix/main.cf && postconf max_idle
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
postconf: warning: /etc/postfix/main.cf, line 1: overriding earlier entry: max_idle=600s
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
postconf: warning: /etc/postfix/main.cf, line 1: overriding earlier entry: max_idle=600s
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
postconf: warning: /etc/postfix/main.cf, line 1: overriding earlier entry: max_idle=600s
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
postconf: warning: /etc/postfix/main.cf, line 1: overriding earlier entry: max_idle=600s
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
postconf: warning: /etc/postfix/main.cf, line 1: overriding earlier entry: max_idle=600s
postconf: warning: /etc/postfix/main.cf, line 2: overriding earlier entry: max_idle=300s
max_idle = 600s Adding sleep will reduce the warning spam, but likely isn't contributing anything useful. I would need to test in a different environment to see if that occurs outside of this VM, it may just be disk I/O perf issue and In comparison, there is no issue with $ echo -e "max_idle = 300s\nmax_idle = 600s" >/etc/postfix/main.cf && grep max_idle /etc/postfix/main.cf
max_idle = 300s
max_idle = 600s |
It only helped reduce the warning output spam, not speed up the processing under this assumed I/O stall.
Documentation preview for this PR is ready! 🎉 Built with commit: f1304e4 |
Description
The new way of providing your custom Postfix configuration is similar to
the old way, but now, the whole file is appended at once, and then
"filtered" so Postfix is okay with it.
Users can now override the default Postfix configuration and do not have
to worry about providing the config line-by-line.
The documentation was adjusted accordingly. It emphasises how the
configuration is applied to enable users to understand what they can do
and how they can achieve that.
Note: The new way of adjusting Postfix should in theory be backwards
compatible. (Nothing is ever certain with Postfix...) I had to adjust for
a read-write-conflict with
postconf -nf
though, but this should be fine.I also moved the default compatibility level into the configuration file instead of having it set in the function (where it cannot be properly overridden by the user...).see #2597This should be the last PR for
v11.1.0
:)Fixes #2594
Type of change
Checklist:
docs/
)