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

Mailbox Quotas #1568

Open
wants to merge 59 commits into
base: main
Choose a base branch
from
Open

Mailbox Quotas #1568

wants to merge 59 commits into from

Conversation

jrsupplee
Copy link

This is an implementation of mailbox quotas using dovecot's quota capabilities. This pull request supersedes a previous pull request that contained some changes not pertinent to the main repository.

* add quota column
* modify users_query to return quota_rule
When the `maildirsize` file does not exist it causes the script to fail.
The IOError is not caught by the execpt
@ctrl-i ctrl-i mentioned this pull request Jun 19, 2019
Copy link
Contributor

@dexbleeker dexbleeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! This is very useful.

I do have some minor feedback, however.
And a question: Is it possible for users to view the usage percentage (in Roundcube)?

@@ -787,7 +788,7 @@ def get_latest_miab_version():
from socket import timeout

try:
return re.search(b'TAG=(.*)', urlopen("https://mailinabox.email/setup.sh?ping=1", timeout=5).read()).group(1).decode("utf8")
return re.search(b'TAG=(.*)', urlopen("https://raw.githubusercontent.com/jrsupplee/mailinabox/master/setup/bootstrap.sh", timeout=5).read()).group(1).decode("utf8")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right.

@@ -805,11 +806,11 @@ def check_miab_version(env, output):
latest_ver = get_latest_miab_version()

if this_ver == latest_ver:
output.print_ok("Mail-in-a-Box is up to date. You are running version %s." % this_ver)
output.print_ok("Mail-in-a-Box with quota support is up to date. You are running version %s." % this_ver)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed in the PR right? I understand this is useful when running your fork, but not in the 'official' MIAB.

elif latest_ver is None:
output.print_error("Latest Mail-in-a-Box version could not be determined. You are running version %s." % this_ver)
else:
output.print_error("A new version of Mail-in-a-Box is available. You are running version %s. The latest version is %s. For upgrade instructions, see https://mailinabox.email. "
output.print_error("A new version of Mail-in-a-Box is available. You are running version %s. The latest version is %s. For upgrade instructions, see https://github.com/jrsupplee/mailinabox/blob/master/README.md. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also does not seem right.

@JoshData
Copy link
Member

JoshData commented Mar 8, 2020

@dexbleeker left some important comments that need to be addressed.

We also never include full configuration files that clobber existing package configuration files. This PR adds three new configuration files that clobber existing ones. Those need to be removed and any settings changes they make to stock Dovecot configuration files would need to be made using our editconf tool or another method we use for other files we modify.

@jrsupplee
Copy link
Author

@dexbleeker left some important comments that need to be addressed.

We also never include full configuration files that clobber existing package configuration files. This PR adds three new configuration files that clobber existing ones. Those need to be removed and any settings changes they make to stock Dovecot configuration files would need to be made using our editconf tool or another method we use for other files we modify.

Will quotas be integrated if these are addressed?

@jrsupplee
Copy link
Author

And a question: Is it possible for users to view the usage percentage (in Roundcube)?

Yes

@JoshData
Copy link
Member

Will quotas be integrated if these are addressed?

I can't promise to merge something I haven't seen, and I could have more requests, but I don't foresee a problem.

@JoshData
Copy link
Member

The merge conflicts need to be resolved also.

@jrsupplee
Copy link
Author

@dexbleeker left some important comments that need to be addressed.

We also never include full configuration files that clobber existing package configuration files. This PR adds three new configuration files that clobber existing ones. Those need to be removed and any settings changes they make to stock Dovecot configuration files would need to be made using our editconf tool or another method we use for other files we modify.

Which files are getting clobbered? If there are files that are copied it is because they did not previously exist in the installation.

@jrsupplee
Copy link
Author

Will quotas be integrated if these are addressed?

I can't promise to merge something I haven't seen, and I could have more requests, but I don't foresee a problem.

I understand that you need to see the final product, but it will be essentially what you see here with some updates.

I stopped trying with the quotas pull request because you (@JoshData) made a remark that you were not sure that quotas should be a part of the master repository. So has a decision been made to integrate quotas or is it still under discussion?

@JoshData
Copy link
Member

JoshData commented May 8, 2021

Which files are getting clobbered?

20-imap.conf and 90-quota.conf. 20-imap.conf is edited elsewhere in our setup so it can't also be clobbered. 90-quota.conf appears to be essentially empty (no non-comment lines) in the distribution-supplied file so clobbering may be ok, but to avoid apt upgrade conflict issues it may be better to create a new file.

So has a decision been made to integrate quotas or is it still under discussion?

I am OK to merge if the feedback is addressed, yes. In the years since this began people have used your PR and proven that it is a good solution - that's what changed.

@Derridaralalala
Copy link

@jrsupplee any timeline on this one? it would be awesome! thanks so much.

@guizmoau
Copy link

How can one help to get that merged ?

Shall one fork the fork, address the feedback and...pull request to merge ? :/

The feedback that needs to be addressed is about 20-imap.conf that is "clobbered" ? How do we un-cloberred such ?

@myfirstnameispaul
Copy link
Contributor

@guizmoau I know when I run updates, if the update changes anything in /etc/postfix/main.cf, the install script politely places a # on my customizations. Maybe look at how that is done, as I understand this is what the non-clobber policy to be referring to, but I may be misunderstanding that term.

@JoshData
Copy link
Member

Yes, that's what I'm referring to.

@sorokaalex
Copy link

Hello @JoshData , is there any change get quota working under official MiaB release? appreciate your working on that. thank you.

@JoshData
Copy link
Member

JoshData commented Nov 2, 2022

I am not working on it. If someone completes this PR addressing my feedback, then I'll happily merge it.

@sorokaalex
Copy link

I am not working on it. If someone completes this PR addressing my feedback, then I'll happily merge it.

thank you per your reply. could you please inform us what we need to do to complete PR?

@sorokaalex
Copy link

Hello @jrsupplee and @dexbleeker I saw you have been working on this subject for some years ago and this PR still open. Is there anything I can help to complete this PR to Josh be able to merge it in to official release ? thank you

@chadfurman
Copy link

Temperature check: if I were to pick this up, are we still interested in getting it merged? I can update it to the latest code and address changes etc. TBD on timeline.

@chadfurman
Copy link

I read through the code in this PR, it seems pretty straight forward. The bootstrap script is weird, I agree, so I'll take a look at that. I'll make a new PR that includes these changes after I get the basic app running locally and try to understand what's changed in the main codebase over the last four years relative to this.

Specifically on the last point, if anyone has any thoughts here please lmk. I can't see the conflicts for some reason on github, so it won't be till I pull it down and run the diff that I see the full extent of the challenge with updating this code to the latest version.

@kiekerjan
Copy link
Contributor

There happened to be a discussion on the forum a few days back. Perhaps you saw it?
In the end you need buy-in from joshdata, but it looks like he is on board with this change.

In addition, the powerMiaB fork includes the quota branch, it might be of help if both branches have not diverged too much.

@chadfurman
Copy link

I'm working with stylnchris, actually. Thank you for the links. I can take a look at PMiaB also, though I suspect my effort will be best placed looking at the current MiaB code and forward-porting the changes in this PR (plus the requested edits). I'd love to hear from @JoshData as getting this merged to main-line would be ideal. Either way, I'll make a new PR with the latest code from this repo mixed with the code from this branch and link it here when it's ready. I'll be picking this work up later this month.

@chadfurman
Copy link

chadfurman commented Apr 27, 2024

New PR here with conflicts resolved and updated to latest MiaB:

#2387

All code review feedback addressed. A couple items I saw while working on it that I have questions about but otherwise should be ready for re-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants