Fix code in Basic01 #1249
Merged
Fix code in Basic01 #1249
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
tgreenx
added
T-Bug
tgreenx
requested review from
mattias-p,
matsduf,
hannaeko,
a user and
marc-vanderwal
|
I will draft this PR for now as I will add more changes, unrelated to the mentioned issue. |
tgreenx
force-pushed
the
fix-basic01
branch
2 times, most recently
from
9df575a to
bd7d10e
Compare
This PR is now ready for review. |
|
@matsduf Github checks are not running it seems, I'm not sure on how to trigger them manually. I think I need to repush something in the PR for that to happen. Maybe you know of another way. |
CI checks will only start if the branch name matches the following as far as I can see: |
|
Now the release branch has been renamed, and CI will run. Push an empty commit to the branch to trigger CI to run. |
These changes are to align the code with the specification. - Update and add missing code (step 7.3 of the specification - relates to issue zonemaster#1248) - Add missing conditional checks for the start of the Basic module and test cases ("scope" section of the specification") - Refactoring - Update unitary tests
marc-vanderwal
approved these changes
Jul 12, 2023
matsduf
approved these changes
Jul 12, 2023
tgreenx
added a commit
to tgreenx/zonemaster-engine
that referenced
this pull request
Aug 2, 2023
Commit f1354e1 (zonemaster#1249) introduced a bug (regression) when comparing domain names. It was using the function 'index()' to make such a comparison, on names that were no longer in their FQDN form. Thus, any domain name beginning with the characters of its TLD (e.g. 'norid.no') would wrongly be discarded. But even if that bug was reverted, it could still be problematic for multi-level zones, e.g. 'no.norid.no'. So, instead of relying on a pure Perl function that does string comparison, in this commit we make use of the Zonemaster::Engine::DNSName class, specifically its function 'common()', to count the common labels between domain names.
tgreenx
added a commit
to tgreenx/zonemaster-engine
that referenced
this pull request
Aug 2, 2023
Commit f1354e1 (zonemaster#1249) introduced a bug (regression) when comparing domain names. It was using the function 'index()' to make such a comparison, on names that were no longer in their FQDN form. Thus, any domain name beginning with the characters of its TLD (e.g. 'norid.no') would wrongly be discarded. But even if that bug was reverted, it could still be problematic for multi-level zones, e.g. 'no.norid.no'. So, instead of relying on a pure Perl function that does string comparison, in this commit we make use of the Zonemaster::Engine::DNSName class, specifically its function 'common()', to count the common labels between domain names.
Merged
tgreenx
added a commit
to tgreenx/zonemaster-engine
that referenced
this pull request
Aug 2, 2023
Commit f1354e1 (zonemaster#1249) introduced a bug (regression) when comparing domain names. It was using the function 'index()' to make such a comparison, on names that were no longer in their FQDN form. Thus, any domain name beginning with the characters of its TLD (e.g. 'norid.no') would wrongly be discarded. But even if that bug was reverted, it could still be problematic for multi-level zones, e.g. 'no.norid.no'. So, instead of relying on a pure Perl function that does string comparison, in this commit we make use of the Zonemaster::Engine::DNSName class, specifically its functions 'common()' and 'is_in_bailiwick', to count and compare the labels composing domain names.
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 fixes the implementation of Basic01 by adding missing code due to an oversight during the initial implementation update (#1212).
It also adds conditional checks to the start of the Basic module and Test Cases (see "Scope" section of Basic01) that were also missed.
Context
Fixes #1248
Changes
How to test this PR
Check that this zone (from #1248) works correctly. Note that we do not have an unitary test for that kind of zone configuration yet.
Check that a non existing zone, normal test makes Zonemaster:
Check that a non existing zone, undelegated test makes Zonemaster: