I am informed that this makes the ARM64 build fail. I'll look into it.

I am informed that this makes the ARM64 build fail. I'll look into it.

And I probably should have checked the test suite before diving into a review 😰 (I think it took me roughly 2 hours)

Here's the ARM64 failure:

Unable to locate package dovecot-lua

This is one of those maintenance concerns I raised initially. The Debian package doesn't have an ARM64 variant available. Actually it seems the package name is wrong, where did you get dovecot-lua from? It seems to be dovecot-auth-lua (has full platform support)?

The other test failure with Fail2Ban is unrelated, that's been acting up lately for unknown reasons.

I'm a bit surprised though, as for some reason our AMD64 build was successful. It should not have used a cache due to packages.sh being updated...

This COPY for packages.sh has been treated as CACHED for some reason 🤔

Not going into all of your extensive review points of the documentation at the moment. They are good points, and I will update the documentation. 😄

I am informed that this makes the ARM64 build fail. I'll look into it.

And I probably should have checked the test suite before diving into a review 😰 (I think it took me roughly 2 hours)

Here's the ARM64 failure:

Unable to locate package dovecot-lua

This is one of those maintenance concerns I raised initially. The Debian package doesn't have an ARM64 variant available. Actually it seems the package name is wrong, where did you get dovecot-lua from? It seems to be dovecot-auth-lua (has full platform support)?

tools/scripts/build/packages.sh imports the Dovecot deb community repo. This repository provides only x86_64 packages, and it uses package name dovecot-lua for Dovecot's Lua support (dovecot-auth-lua is just a dummy package in this repository, which is the reason it is available for all architectures).

If building on ARM64, the Debian repositories are used since Dovecot's community repository is unable to provide ARM64 packages. Debian repositories use package name dovecot-auth-lua for Dovecot's Lua support.

I will change packages.sh so that Dovecot's community repository is not used when not compiling for x86_64, and I will make it use the right Lua package name for each situation.

The other test failure with Fail2Ban is unrelated, that's been acting up lately for unknown reasons.

Thanks for the notification. I'll ignore these test failures if they pop up for me.

tools/scripts/build/packages.sh imports the Dovecot deb community repo. This repository provides only x86_64 packages, and it uses package name dovecot-lua for Dovecot's Lua support

Ah thanks for that clarification.

I will change packages.sh so that Dovecot's community repository is not used when not compiling for x86_64, and I will make it use the right Lua package name for each situation.

I have a bad habit of replying too eagerly and didn't read this far 😑 Well that's sorted then 👍

The only reason we use the community repo for Dovecot AFAIK, was due to needing a version newer than Debian 11 provided. While another maintainer is reluctant to use the Debian 12 Dovecot release as the version is behind the community repo, but Debian 12 won't be supported by the community repo until Dovecot 2.4/3.0 is released AFAIK.

I read this as that there is a Dovecot version difference between DMS on x86_64 and ARM64. After some testing, it turns out this is true.

x86_64: Dovecot 2.3.21 (from the community repo)
ARM64: Dovecot 2.3.13 (from Debian's repo)

I do agree that this is quite a conundrum if stability is preferred and you wish to avoid community repos. Still, if you consider the rspamd community repository for Fedora, they do provide ARM64 packages in their rpm repositores, unlike Dovecot (rspamd's documentation seems to be lagging behind on this). That would at least take care of the software version discrepancy between architectures, and any headaches that might cause.

I have a bad habit of replying too eagerly and didn't read this far 😑 Well that's sorted then 👍

All is forgiven. Please provide the same courtesy if I do that at some point (which I try to avoid, but no guarantees!). 😄

I'll keep working on this PR in the coming few days.

I do not have the time to read through the whole thread, so here are my two cents:

The version discrepancy of Dovecot between amd64 and arm64 is similar to Rspamd, and not an issue if you ask me. It is not perfect, and I try to keep the differences for Rspamd at a minimum. We should not care about this at the moment if you ask me.

About the newly added package: this may sound harsh, but I really don't see why this could not be done with user-patches.sh. I really appreciate the Documentation effort, and I think we should merge that 🚀 But I see bigger maintenance issues on the horizon with the package addition. So, can we maybe only merge an adjusted version of the new documentation?

@georglauterbach

I really don't see why this could not be done with user-patches.sh.

This was explained in their response prior to two maintainer approvals: #3575 (comment)

_ All I did was add dovecot-lua to the package list in packages.sh (and disable my user-patches.sh, of course). Lua support in Dovecot works immediately.
I would not like to use user-patches.sh, since the deb packages in the image do not have to be in sync with the deb packages in Dovecot's repository.

Previously they had been using user-patches.sh, but the concern was due to third-party repo not being in sync with image release. Since we don't regularly update any tags beyond :edge due to current CI infrastructure this is a valid concern while the Dovecot community repo remains the default in builds.

In my docs review I have pointed out that they may need to rely on user-patches.sh in future for other changes to reliably be applied, as replacing entire Dovecot config files for small changes is not advisable, especially when it's a custom config we override with file replacement ourselves (as has been problematic for some users when we changed the Dovecot SASL auth socket file path).

But the package concern AFAIK is valid to warrant inclusion into the image instead of user-patches.sh.

I see bigger maintenance issues on the horizon with the package addition.

As per the issue link with maintainer approvals and my docs review feedback above, maintainers have been very clear that @Zepmann is responsible for resolving any related maintenance issues that arise with the inclusion of this package.

If it causes us any problems that are not resolved to maintainer satisfaction in a timely manner, the package will be dropped in a future release. This is the stance for any community contributed packages / features that are deemed niche like fetchmail and getmail (which also lack thorough testing).

To further take that stance, I've also suggested like the debug group in packages.sh we do something similar for community contributed packages, so that the distinction for maintenance is clear.

I have updated the documentation based on comments from @polarathene. Furthermore, I changed packages.sh so that the Dovecot community repository is only used when relevant (i.e. the platform is x86_64). This fixes the ARM build issue.

Everything works with the current development version of DMS. As discussed, there will be changes to DMS in the future that will break the example in the documentation. I think it is best to start with a new PR when that time comes. This is a community effort, and as such it is no issue if it lags behind official development and breaks sometimes. The documentation also notes this as a warning for any users who want to use Lua-based authentication.

This PR is not forgotten. I'll pick it up later this week.

I've marked this for v13.1.0. The next release is v13.0.0, so this PR will not block on v13.0.0

Finally had some time to work on this!

I think most of the comments have been solved adequately. If not, please let me know.

Also, the Lua documentation was simplified somewhat. By configuring Nextcloud (as the server) better, it is not necessary to test in Lua (as the client) if the provided password is a Nextcloud application password. The relevant Nextcloud configuration parameters are also included now in the documentation.

Finally, I am considering adding this documentation to the examples section instead of as a main page in the advanced configuration section, but I do not know if this would make the review process harder. Therefore, please review this first, but do not merge the PR if you think it is acceptable. Instead, let me know your comments and I'll make another change to move the documentation to the proper section.

I am considering adding the documentation to the 'Use Cases' subsection of Examples. This will take the page out of the more important parts of the documentation, making it less critical if it lags behind a bit with the current state of DMS.

Most critical for this PR remains the added Lua support for Dovecot. This support is now explicitly marked as coming from the community, to make it stand out from the 'core' development of DMS. I expect that it will not be difficult to keep this support, since it is not hard to enable Lua support with any method used to build/install Dovecot. However, this depends on future versions of Dovecot keep playing nice.

Please notify me if Dovecot Lua support is ever considered problematic or a feature to be removed. This way, I can add my $ 0.02 and maybe help by solving the problem or by thinking of alternatives.

I am considering adding this documentation to the examples section instead of as a main page in the advanced configuration section, but I do not know if this would make the review process harder.
I am considering adding the documentation to the 'Use Cases' subsection of Examples

Yes, this sounds like a good choice 👍

Thanks for waiting for review before making the change, I had been thinking of relocating it from my initial review but I didn't want to deal with any added review friction :)

Great work, thanks for the contribution to DMS! ❤️

Please notify me if Dovecot Lua support is ever considered problematic or a feature to be removed. This way, I can add my $ 0.02 and maybe help by solving the problem or by thinking of alternatives.

Will do 👍

Thanks for the extensive feedback and an open mind, @polarathene. I'm looking forward to working together again in the future.

Currently I'm busy with some other stuff. I'll move the documentation page later today (as in 'within 12 hours from now', but I'll do it ASAP within that timeframe).

For expectation management, is this PR to be included in release v13.0.0 or v13.1.0, considering the timeline and the v13.1.0 milestone?

For expectation management, is this PR to be included in release v13.0.0 or v13.1.0, considering the timeline and the v13.1.0 milestone?

If this is to be merged in the next few days, it's going to be in v13.0.0. When @polarathene merges this PR, they can just adjust the milestone.

Moved the documentation. I have nothing more to add for this PR unless something new pops up. In that case, let me know! 😃

Documentation preview for this PR is ready! 🎉

Built with commit: 1f703a8