Fixing errors display when using options '--ns' or '--ds' #382
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
MichaelTimbert
requested review from
tgreenx,
mattias-p,
matsduf and
marc-vanderwal
MichaelTimbert
added
the
V-Minor
tgreenx
reviewed
Aug 20, 2024
|
This PR is based on develop branch before the last release. I suggest that it is rebased on latest master or develop branch. |
matsduf
reviewed
Aug 21, 2024
matsduf
previously requested changes
Aug 28, 2024
matsduf
reviewed
Aug 28, 2024
matsduf
previously approved these changes
Aug 28, 2024
|
@MichaelTimbert, can you please handle the conflicts? |
I have extracted CLI argument verification from 'add_fake_delegation' and 'add_fake_ds' to 'check_fake_ds' and 'check_fake_delegation'. check_* functions are called before displaying the header and only displays CLI argument errors. add_* functions are called after the header and throw errors from the Engine.
Code formatting Co-authored-by: tgreenx <[email protected]> Co-authored-by: Marc van der Wal <[email protected]>
Remove unused variables Co-authored-by: tgreenx <[email protected]>
Improve code Co-authored-by: tgreenx <[email protected]>
Move --ns & --ds validation earlier in the code Co-authored-by: tgreenx <[email protected]>
Improve code and regex validation Co-authored-by: Marc van der Wal <[email protected]>
Reuse code from Zonemaster::Engine for IP validation Co-authored-by: Mats Dufberg <[email protected]>
Better hint for --ds option Co-authored-by: Mats Dufberg <[email protected]>
marc-vanderwal
previously approved these changes
Oct 21, 2024
matsduf
requested changes
Oct 21, 2024
There was a problem hiding this comment.
# zonemaster-cli --show-testcase --no-ipv6 --test Basic zonemaster.net --ns ns1.zonemaster.net --level info
1.11 ERROR Unspecified The fake delegation of domain zonemaster.net includes an in-zone name server ns1.zonemaster.net without mandatory glue (without IP address).
\
Seconds Level Testcase Message
======= ======== ============== =======
0.00 INFO Unspecified Using version v6.0.0 of the Zonemaster engine.
0.00 INFO Basic01 The zone "zonemaster.net" is found.
0.00 INFO Basic01 This is a test of an undelegated domain so finding the parent zone is disregarded.
0.16 CRITICAL Basic02 There is no working name server for "zonemaster.net" so it is unreachable.
0.16 ERROR Basic02 Name server "ns1.zonemaster.net" cannot be resolved into an IP address.
Why is the first message before the header?
I mistakenly moved the |
matsduf
approved these changes
Oct 22, 2024
marc-vanderwal
approved these changes
Oct 22, 2024
|
Release testing for 2024.2: When trying the first command, the output doesn’t report any errors, although it should: All other command lines listed in the testing procedures show an error, without showing the “seconds, level, testcase, message” header at all, so I assume this works as expected. I’ve created issue #402 to track this problem. |
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 separate CLI errors from Engine errors.
Errors on CLI argument are catch before displaying the header.
Context
Fixes #358
Changes
I have split CLI argument verification from Engine settings.
I have also add regex validation for '--ds' arguments
How to test this PR
Stress test zonemaster-cli with various '--ns' and '--ds' arguments.
Errors from command line input validation must be display before any header.
Errors from the Engine must be formatted correctly after the header.
here some example to cmd to run: