Skip to content

Minor changes to Zone11 msgids#1264

Merged
tgreenx merged 2 commits into
zonemaster:developfrom
marc-vanderwal:bugfix/zone11-msgid-consistency
Jun 11, 2024
Merged

Minor changes to Zone11 msgids#1264
tgreenx merged 2 commits into
zonemaster:developfrom
marc-vanderwal:bugfix/zone11-msgid-consistency

Conversation

@marc-vanderwal

Copy link
Copy Markdown
Contributor

Purpose

This PR brings the msgids defined in the specification for Zone11 closer to what would be implemented in Engine.

Context

See zonemaster/zonemaster-engine#1328.

Changes

Slight changes to the msgids in Zone11.

How to test this PR

Not applicable.

@tgreenx tgreenx requested review from matsduf, mattias-p and tgreenx May 16, 2024 16:01
@tgreenx tgreenx added A-TestCase Area: Test case specification or implementation of test case A-Documentation Area: Documentation only. labels May 16, 2024
@tgreenx tgreenx added this to the v2024.1 milestone May 16, 2024
Comment thread docs/public/specifications/tests/Zone-TP/zone11.md Outdated
Comment on lines +47 to +49
Z11_SPF1_MULTIPLE_RECORDS | ERROR | ns_ip_list | The following name servers returned more than one SPF policy. Name servers: {ns_ip_list}.
Z11_SPF1_SYNTAX_ERROR | ERROR | ns_ip_list | The zone’s SPF policy has a syntax error. Policy retrieved from the following nameservers: {ns_ip_list}.
Z11_SPF1_SYNTAX_OK | INFO | | The zone’s SPF 1 policy has correct syntax.

@tgreenx tgreenx May 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the version is omitted in the message id, would it make sense to rename the message tags too in that regard?
e.g. Z11_SPF1_SYNTAX_OK to Z11_SPF_SYNTAX_OK (and others)

This might not be worth the hassle, though. I'm fine with either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renaming the message tags will be too disruptive, in my opinion. I suggest we leave them be.

Bring the msgids closer to what would be implemented in Engine.
@marc-vanderwal marc-vanderwal force-pushed the bugfix/zone11-msgid-consistency branch from efd218e to 29694a3 Compare May 21, 2024 07:47
@marc-vanderwal marc-vanderwal requested a review from tgreenx May 21, 2024 07:47
tgreenx
tgreenx previously approved these changes May 21, 2024
Comment on lines +48 to +50
Z11_SPF1_SYNTAX_ERROR | ERROR | ns_ip_list | The zone’s SPF policy has a syntax error. Policy retrieved from the following nameservers: {ns_ip_list}.
Z11_SPF1_SYNTAX_OK | INFO | | The zone’s SPF policy has correct syntax.
Z11_UNABLE_TO_CHECK_FOR_SPF | ERROR | | None of the zone’s name servers responded with an authoritative response to queries for SPF policies.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be strict, the policy is a feature of a name (mail domain) not a zone. In a zone you could have multiple SPF policies for different mail domains, can't you. E.g. in the zone test.xa you could have one for afnic.test.xa and another for iis.test.xa given that those are used for mail addresses, e.g. info@afnic.test.xa and info@iis.test.xa, respectively. Zonemaster only tests the SPF policy of the mail domain equal to the zone apex, but that is still just a name.

I suggest something like "The SPF policy of the zone apex (...)"

(I also put the same comment in zonemaster/zonemaster-engine#1348 (review))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or maybe something like "The SPF policy of {domain} has (...)" where "{domain}" is the name of apex. It will probably be easier to understand than my first suggestion,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I’ll do that. However, adding the {domain} argument will require a database migration script in order not to break existing messages, because they do not have any arguments.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is something that we have not considered in the past, but it is not irrelevant. However, if we have that requirement it will be hard to update the messages in the future. When we remove a tag (or rename) it will affect even more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, database migrations are annoying, but I have another PR open that could use a similar script for the same reason. We could piggyback on that one if we want.

Technically, any domain, not just zones, can have SPF policies.
Zonemaster will only fetch the domain under test’s SPF policy. This
means that language referring to “the zone’s SPF policy” is technically
incorrect.

This commit updates the following messages tags to remove such language:

 * Z11_INCONSISTENT_SPF_POLICIES;
 * Z11_NO_SPF_FOUND;
 * Z11_SPF1_SYNTAX_ERROR;
 * Z11_SPF1_SYNTAX_OK.

The three last tags in the list get a new argument, {domain}, which is
necessary for the msgid to stay meaningful and user-friendly. The
alternative would be to say something like “the SPF policy at the zone’s
apex”, but “the SPF policy at domain.example” is much better. This is,
however, a change that should ideally be addressed by a database
migration script so that old tests stay meaningful.
@marc-vanderwal

Copy link
Copy Markdown
Contributor Author

I’ve changed a few more msgids to completely remove language such as “the zone’s SPF policy” in the messages this test case generates. @matsduf, what do you think about the new messages?

@marc-vanderwal marc-vanderwal requested a review from tgreenx May 24, 2024 11:22
tgreenx added a commit to tgreenx/zonemaster-engine that referenced this pull request Jun 11, 2024
@tgreenx

tgreenx commented Jun 11, 2024

Copy link
Copy Markdown
Contributor

Implemented in zonemaster/zonemaster-engine#1348

@tgreenx tgreenx merged commit d9164af into zonemaster:develop Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Documentation Area: Documentation only. A-TestCase Area: Test case specification or implementation of test case

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implemented msgid for Zone11 do not match the messages in specification

3 participants