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

feature: adding getmail as an alternative to fetchmail #2803

Merged

Conversation

LucidityCrash
Copy link
Contributor

@LucidityCrash LucidityCrash commented Sep 29, 2022

Description

Adding getmail as an alternative to Fetchmail

Fixes #2513

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • [?] My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • [?] If necessary I have added tests that prove my fix is effective or that my feature works
  • [?] New and existing unit tests pass locally with my changes

all of no_container.bats show fails
test_helper.bats has a fails:
✗ wait_for_smtp_port_in_container returns immediately when port found in 10400ms [10400]
318 tests, 9 failures, 4 skipped in 2025 seconds
I don't believe these are to do with my additions.

I've added a stub for testing but since getmail doesn't run as as daemon like fetchmail I'm not sure how to test.
Added some docs and basic config examples.

Not sure that putting the [options] section into a seperate file that is appended to each of the fetch configs as you might actually want different configs for different sources.

Note I'm downloading the sid version of getmail6 as the stable one is quite a bit out of date.

@georglauterbach georglauterbach added this to the v12.0.0 milestone Sep 29, 2022
@georglauterbach georglauterbach added kind/new feature A new feature is requested in this issue or implemeted with this PR meta/feature freeze On hold due to upcoming release process area/features labels Sep 29, 2022
LucidityCrash added a commit to LucidityCrash/docker-mailserver that referenced this pull request Oct 1, 2022
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Partial review as the browser crashed, but this was salvaged thankfully.

docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Remainder of review completed 😀

target/bin/debug-getmail Outdated Show resolved Hide resolved
target/scripts/build/packages.sh Outdated Show resolved Hide resolved
target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
target/scripts/wrapper/getmail-wrapper.sh Outdated Show resolved Hide resolved
test/mail_getmail.bats Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

I used this online crontab tool, which should link to an example of every 29th minute. You'll see that it's only within an hour, then it resets. So every 50th minute would be always at 50 minutes past the current hour, technically every hour at that point.. same with every 31 minutes.

Thus I think the actual max is 30? (Every 30 minutes) Otherwise it's effectively hourly from then on, and anything after 60 likewise ends up being every hour (just with no offset).

I don't know cron that well, the other maintainers can provide feedback on that.


I'm going to batch commit the suggestions, so only the remaining feedback after (3 items left) that needs to be addressed 👍

docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
mailserver.env Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/environment.md Outdated Show resolved Hide resolved
@georglauterbach georglauterbach modified the milestones: v12.0.0, v11.3.0 Oct 11, 2022
Copy link
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

Looks very good, just some minor stylistic issues. I hope you can just batch-commit my suggestions :)

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved
target/bin/getmail-cron Outdated Show resolved Hide resolved
@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Oct 13, 2022
@georglauterbach
Copy link
Member

This can now be merged when the comments are resolved :)

@georglauterbach
Copy link
Member

Applied my own review feedback as well to make the final reviews faster :)

Copy link
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

From a scripts perspective, LGTM. I hope all tests pass now :)

Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

I will batch commit these changes.

Fixes some typos, and uses permalinks instead of referencing lines on the master branch which is not a stable reference we can rely on.

docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
docs/content/config/advanced/mail-getmail.md Outdated Show resolved Hide resolved
polarathene
polarathene previously approved these changes Oct 14, 2022
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

We still may want to consider tests.

If I understand this feature correctly. We could test it by having two DMS containers running, and treating one of them as the remote mail-server (the role of gmail for example)?

Additionally, here is the getmail6 bats test file that uses DMS to handle it's testing. I haven't looked through it properly yet.

@georglauterbach
Copy link
Member

Yes, I'd like to see some tests as well - good idea :)

@polarathene polarathene self-assigned this Oct 17, 2022
@polarathene
Copy link
Member

I've assigned myself, but welcome anyone else to tackle the remaining task of getting some tests sorted.

This is a low priority for me and may not be something I can work on until December.

@github-actions github-actions bot added the meta/stale This issue / PR has become stale and will be closed if there is no further activity label Nov 6, 2022
@LucidityCrash
Copy link
Contributor Author

LucidityCrash commented May 14, 2023

Actually found a few minutes ... think I got it this time :

ok 18 [Getmail] default configuration exists and is correct in 350ms
ok 19 [Getmail] debug-getmail works as expected in 993ms

and the tests in my fork are green :)

@georglauterbach
Copy link
Member

Looks good! I will have a look tomorrow for final review 🚀

Copy link
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

Looks good to me now :) Maybe @casperklein want to have another look at this as well, and I think there is one comment from him that should also be resolved. But overall, nice work 🚀

docs/content/config/environment.md Outdated Show resolved Hide resolved
mailserver.env Outdated Show resolved Hide resolved
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Copy link
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

Just some small change requests. Otherwise ready to merge.

docs/content/config/environment.md Outdated Show resolved Hide resolved
target/scripts/build/packages.sh Show resolved Hide resolved
target/scripts/build/packages.sh Outdated Show resolved Hide resolved
target/scripts/start-mailserver.sh Outdated Show resolved Hide resolved
target/scripts/startup/setup.d/getmail.sh Outdated Show resolved Hide resolved
Co-authored-by: Casper <casperklein@users.noreply.github.com>
@georglauterbach
Copy link
Member

@casperklein I went ahead and applied your feedback at this point. If all checks pass, I will take care of merging this (so we can get it into v12.2.0/v13.0.0).

@georglauterbach
Copy link
Member

It seems I can't merge because of the request for changes that's still blocking 🫠 I could circumvent this as an administrator, but I won't 😂 I enabled auto-merge, so as soon as you approve @casperklein this PR will be merged.

@georglauterbach georglauterbach enabled auto-merge (squash) May 23, 2023 14:53
@georglauterbach georglauterbach enabled auto-merge (squash) May 23, 2023 14:54
@github-actions
Copy link
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 38d9adb

@georglauterbach georglauterbach merged commit 7af7546 into docker-mailserver:master May 23, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/features kind/new feature A new feature is requested in this issue or implemeted with this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] getmail instead/as well as fetchmail
5 participants