chore(madmail): port to v2 - #31
Conversation
7f281db to
54ec1b1
Compare
6c9a088 to
d1f42b0
Compare
17fa180 to
aa1e17d
Compare
cbd5c12 to
756c948
Compare
756c948 to
8d3969e
Compare
aa1e17d to
a726488
Compare
a726488 to
02a4d4d
Compare
hpk42
left a comment
There was a problem hiding this comment.
please
-
drop the @latest dynamic git resolving logic (does network requests during CLI parsing and is orthogonal issue to madmail v2), just default to @main or latest release tag for now.
-
Keep the go->rust madmail upgrade, and also support for downloading prebuilt binaries, the imap-test fix to work against madmail-v2 is fine as well.
-
Prepare a draft themadorg/madmail PR that uses this branch here. If i am not mistaken you can pick up this PR branch here with something like
jobs:
lxc-test:
name: cmlxc deploy and run tests
uses: chatmail/cmlxc/.github/workflows/lxc-test.yml@main
with:
cmlxc_version: j4n/madmail-main
cmlxc_commands: |
cmlxc init
cmlxc deploy-madmail --source ./ mad0
cmlxc deploy-cmdeploy --source @main relay0
cmlxc test-mini mad0 relay0
cmlxc test-mini relay0 mad0
cmlxc test-madmail mad0|
Thank you. I have addressed your feedback, and updated PR body text accordingly; madmail PR is themadorg/madmail#144. |
hpk42
left a comment
There was a problem hiding this comment.
thanks for addressing most of the feedback!
However, the v0.47.1 pin was intentional, not an oversight, so can not serve as a reason for adding @latest support. What we want i think is:
-
chatmail/relay tests its PR/main against a fixed madmail version.
MadmailDriver.DEFAULT_REFcan stay a fixed tag, just bump it to the current latest madmail v2 one. Relay-tests should automatically use it then, and they will use the already-compiled Rust binary (madmail has tags running ahead of releases, so downloads might more easily fail with@latestbut not sure) -
themadorg/madmail tests its PR/main against chatmail/relay @main (relays are often deployed from main anyway, so madmail should work against such relays, but pinning is also fine)
-
cmlxc's should test against latest pinned relay/madmail or latest main ones or both (but doesn't today, a separate issue)
Despite you fixing the CLI/network issue i still prefer @latest support to come in a separate PR (unstacked! :).
Brought back, pinned to 2.20.0, indeed 2.23.0 is latest tag; thats why I did the HTTP-query; but maybe we can just cache the build somehow at a later point.
Leaving for subsequent edits.
I split it out now, coming up. |
d03252f to
7e15c5a
Compare
hpk42
left a comment
There was a problem hiding this comment.
thanks and sorry for taking a while.
-
PR failed because i had existing (madmail Go) containers.
I think it's better to try to detect and remove the Go-version early on. There aren't too many users of cmlxc but still some, and it will give a weird traceback. I think there should beDriver.cached_checkout_is_staleclassmethod that checksREQUIRED_SOURCE_PATH. This can then be used fromget_git_main_pathto discard the old checkout. -
PR description says "Fetch tags so RELAY_REF and --source can checkout release tags" but i think you can still just do "git fetch origin" and it autofollows the tags (madmail tags from main)
It' helpful for review if all lines in a PR are neccessary.
881fead to
2faa771
Compare
|
Thanks for re-reviewing. I applied your suggestions, they were correct.
Ah, good point. I added a commit that addresses this, separatly, for easier review, with a simple test to check it but I haven't tested with stale go containers yet.
Indeed, apologies. |
|
i think the timeout 30 -> 60 was really about allowing CI to finish, and you removed it in force-push. please also squash everything to a single commit, and make sure CI passes. It's getting ready for merge. |
- Drop v1 support entirely rather, set DEFAULT_REF to v2.23.2, latest release.
- Replace Go toolchain block with rustup, installed if missing or below
Cargo.toml's requirement.
- Web admin UI build is handled by the main build process now
- driver_base.py: pass tag through on_init_relay() for driver consumption
- madmail's on_init_relay now tries a download first for a semver release tags,
falls back to the source build.
- align minitest suite's assertions with madmail v2's behaviour:
- test_delivery_port_blocked: extend grep patterns to current madmail variants.
- test_hide_senders_ip_address: Use FETCH instead of UID SEARCH
unsupported in madmail and scan through all fetched messages.
- run E2E tests from the deployed checkout, not the cached clone
- refactor(driver_base): discard stale checkout from get_git_main_path
Yes, as I wrote in the reply to your question, I checked the times and decided that 10 minutes headroom with the current 30min limit for runs with source-build are enough, so I removed it. Pushed the squashed single commit now. |
This adds support for the v2 Rust-Version and adapt tests; opportunistically download binaries if available to skip the build process:
Support for @latest split into separate PR #46.