Provide implementation for Zone11 (SPF test) #1287
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marc-vanderwal
added
T-Feature
marc-vanderwal
requested review from
mattias-p,
matsduf,
hannaeko,
a user and
tgreenx
marc-vanderwal
force-pushed
the
feature/spf-test
branch
2 times, most recently
from
aa6361f to
b67c363
Compare
|
The CI failures are due to a known issue in Mail::SPF: https://rt.cpan.org/Public/Bug/Display.html?id=34768 |
marc-vanderwal
force-pushed
the
feature/spf-test
branch
2 times, most recently
from
dafa092 to
cab0ca4
Compare
|
Installing |
marc-vanderwal
force-pushed
the
feature/spf-test
branch
3 times, most recently
from
1665d7d to
bc30628
Compare
tgreenx
reviewed
Nov 16, 2023
marc-vanderwal
force-pushed
the
feature/spf-test
branch
2 times, most recently
from
543feac to
ce91cf2
Compare
|
I just need to address your comment on the |
That is now done. |
|
This requires new dependencies, doesn't it? An update of the installation instructions is needed. |
matsduf
previously approved these changes
Nov 27, 2023
marc-vanderwal
added a commit
to marc-vanderwal/zonemaster
that referenced
this pull request
Nov 28, 2023
Pull request zonemaster/zonemaster-engine#1287 adds a run-time dependency on Mail::SPF. This comment documents the change in the public-facing installation instructions. Mail::SPF is not required for building Zonemaster::Engine, so the internal documentation pertaining to the build environment does not need to be updated.
|
Considering Marc's absence and the nature of my suggested change, I took the liberty of providing it, see commit f54b9e8. @matsduf please re-review. |
tgreenx
force-pushed
the
feature/spf-test
branch
2 times, most recently
from
36e858e to
f54b9e8
Compare
tgreenx
previously approved these changes
Nov 28, 2023
Depend on Mail::SPF for validating SPF policies.
There is a long-standing packaging issue with Mail::SPF that prevents cpanm from automatically installing it with non-root privileges. [1] It also seems that upstream is unwilling to patch the problem. Yet this package is very useful for checking SPF syntax. One does not simply reimplement the same thing by oneself. So what are our options? Fork it? Copy the code and maintain it in a Zonemaster::Engine::SPF namespace or similar? Or work around the bug? Fortunately, the Perl library in question is easy to find in distribution package repositories. It’s packaged by Debian, Ubuntu, Alpine Linux, FreeBSD at least. However, in Github Actions, all Perl modules are install from CPAN. That must be because the Perl module packages supplied by the OS are only built against the version of Perl packaged by said OS, and we are testing against multiple different versions of Perl that may be different from the packaged one. In order to keep that consistency, the best option is to install Mail::SPF from CPAN while adding the workaround documented in the library’s INSTALL file. [1]: https://rt.cpan.org/Public/Bug/Display.html?id=34768
In the continuous integration pipeline, use cpanm --sudo instead of plain cpanm to install all Perl packages. Doing so works around a packaging issue with Mail::SPF, and more closely reflects the official installation procedure.
Change the format of the test declarations inside t/Test-zone11.t so that it conforms to the format specified and agreed to in zonemaster#1297. This is unfortunately not a lossless conversion: the old format made it possible to specify extra checks such as “for zone all-different-spf.zone11.xa, the Z11_DIFFERENT_SPF_POLICIES_FOUND should be output 3 times”. Hopefully this can be brought back somehow later. Also, for some reason, the changes in the .t file required a new test run with ZONEMASTER_RECORD=1 set in the environment.
Document function "Zonemaster::Engine::Test::Zone::_spf_syntax_ok()"
tgreenx
force-pushed
the
feature/spf-test
branch
from
f54b9e8 to
9471d82
Compare
tgreenx
approved these changes
Nov 29, 2023
|
Great, thanks! This looks good to me. |
matsduf
approved these changes
Dec 4, 2023
marc-vanderwal
added a commit
to marc-vanderwal/zonemaster
that referenced
this pull request
Dec 4, 2023
Pull request zonemaster/zonemaster-engine#1287 adds a run-time dependency on Mail::SPF. This comment documents the change in the public-facing installation instructions. Mail::SPF is not required for building Zonemaster::Engine, so the internal documentation pertaining to the build environment does not need to be updated.
ghost
mentioned this pull request
tgreenx
added a commit
to tgreenx/zonemaster-cli
that referenced
this pull request
Dec 4, 2023
- Bumped Perl supported versions - Added Mail::SPF dependency. Zonemaster-Engine relies on Mail::SPF and there is a bug preventing its installation from cpanm. See Zonemaster-Engine commit `3ac72b0c` and/or pull request <zonemaster/zonemaster-engine#1287> for more information on this.
tgreenx
added a commit
to tgreenx/zonemaster-cli
that referenced
this pull request
Dec 4, 2023
- Bumped Perl supported versions - Bumped distribution version - Added Mail::SPF dependency. Zonemaster-Engine relies on Mail::SPF and there is a bug preventing its installation from cpanm. See Zonemaster-Engine commit `3ac72b0c` and/or pull request <zonemaster/zonemaster-engine#1287> for more information on this.
matsduf
added
the
S-ReleaseTested
|
Release testing: Looks fine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR introduces an implementation for Zone11, the test case for SPF policies.
Context
Changes
How to test this PR