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.

Sign up for GitHub

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

Jump to bottom

Update Zone09 #1211

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Update Zone09 #1211

merged 2 commits into from
Mar 20, 2023

Conversation

tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Mar 13, 2023 •
edited
Loading

Purpose

This PR proposes a fix to a bug in Zone09. It also adds a missing return value in a message tag.

Context

Fixes #1210

Relates to zonemaster/zonemaster#1128

Changes

  • Dereferencing of array
  • Add "rcode" value to message tag Z09_UNEXPECTED_RCODE_MX

How to test this PR

$ zonemaster-cli --test zone/zone09 smart.ondmarc.com --raw
   0.60 WARNING   Z09_UNEXPECTED_RCODE_MX   ns_ip_list=35.246.222.179; rcode=NOTIMPL

$ zonemaster-cli --test zone/zone09 smart.ondmarc.com
Seconds Level     Message
======= ========= =======
   0.62 WARNING   Unexpected RCODE value (NOTIMPL) on the MX query from name servers "35.246.222.179".

@tgreenx tgreenx added T-Bug Type: Bug in software or error in test case description A-TestCase Area: Test case specification or implementation of test case S-PRforIssue Status: There is a PR that is meant to resolve the issue V-Patch Versioning: The change gives an update of patch in version. labels Mar 13, 2023
@tgreenx tgreenx added this to the v2023.1 milestone Mar 13, 2023
@tgreenx tgreenx requested review from matsduf, hannaeko, a user and marc-vanderwal March 13, 2023 09:32
@tgreenx tgreenx linked an issue Mar 13, 2023 that may be closed by this pull request
Error from code for ZONE09 #1210
Closed
@tgreenx
Copy link
Contributor Author

tgreenx commented Mar 13, 2023 •
edited
Loading

@matsduf I noticed "rcode" value was passed to the message tag but not used, this is due to an oversight in the specification, but I took the liberty of adding it nonetheless since it is still given in the list of arguments. See https://github.com/zonemaster/zonemaster/blob/v2022.2.2/docs/specifications/tests/Zone-TP/zone09.md?plain=1#L115

EDIT: nevermind, there is already a PR to update the specification. zonemaster/zonemaster#1128

@tgreenx tgreenx removed the S-PRforIssue Status: There is a PR that is meant to resolve the issue label Mar 13, 2023
@tgreenx tgreenx mentioned this pull request Mar 13, 2023
Merged
matsduf
matsduf reviewed Mar 13, 2023
View reviewed changes
lib/Zonemaster/Engine/Test/Zone.pm
ns_ip_list => join( q{;}, sort $unexpected_rcode_mx{$rcode} )
ns_ip_list => join( q{;}, sort @{ $unexpected_rcode_mx{$rcode} } )
Copy link
Contributor

Choose a reason for hiding this comment

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

Our current unit tests do not catch this type of errors. I do not say that we should add such tests to the unit tests, but we should be aware of it.

@matsduf matsduf mentioned this pull request Mar 13, 2023
Closed
matsduf
matsduf previously approved these changes Mar 13, 2023
View reviewed changes
@tgreenx tgreenx requested a review from matsduf March 14, 2023 10:15
@tgreenx tgreenx merged commit 25345af into zonemaster:develop Mar 20, 2023
@tgreenx tgreenx deleted the fix-zone09 branch March 20, 2023 14:13
@hannaeko hannaeko self-assigned this Jun 1, 2023
@hannaeko hannaeko added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matsduf matsduf matsduf approved these changes

@hannaeko hannaeko Awaiting requested review from hannaeko

@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

Assignees

@hannaeko hannaeko

Labels
A-TestCase Area: Test case specification or implementation of test case S-ReleaseTested Status: The PR has been successfully tested in release testing T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Milestone
v2023.1
Development

Successfully merging this pull request may close these issues.

Error from code for ZONE09
3 participants
@tgreenx @matsduf @hannaeko