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.

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

Fixing errors display when using options '--ns' or '--ds' #382

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

MichaelTimbert
Copy link

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:

zonemaster-cli --show-testcase --no-ipv6 --test basic/basic01 zonemaster.net --ns 'ns1.zonemaster.net'
zonemaster-cli --show-testcase  example.fr --ns 'ns1.zonemaster..fr.'
zonemaster-cli --show-testcase  example.fr --ns 'ns1.zonemaster.fr/542.0.0.1'
zonemaster-cli --show-testcase  example.fr --ns '/215.4.6.1'
zonemaster-cli --show-testcase afnic.fr --ds "32674 13 2 12CCEEFE3ECD007C"
zonemaster-cli --show-testcase afnic.fr --ds "32674,13,2,NOTHEXADECIMAL"

@MichaelTimbert MichaelTimbert added the V-Minor Versioning: The change gives an update of minor in version. label Aug 19, 2024
@MichaelTimbert MichaelTimbert added this to the v2024.2 milestone Aug 19, 2024
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
@matsduf
Copy link
Contributor

matsduf commented Aug 21, 2024

This PR is based on develop branch before the last release. I suggest that it is rebased on latest master or develop branch.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

The changes are fine. When the other other review comments are handled I can approve.

lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
MichaelTimbert and others added 6 commits August 28, 2024 14:21
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 <96772376+tgreenx@users.noreply.github.com>
Co-authored-by: Marc van der Wal <103426270+marc-vanderwal@users.noreply.github.com>
Remove unused variables

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
Improve code

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
Move --ns & --ds validation earlier in the code

Co-authored-by: tgreenx <96772376+tgreenx@users.noreply.github.com>
MichaelTimbert and others added 2 commits August 28, 2024 14:47
Improve code and regex validation

Co-authored-by: Marc van der Wal <marc.vanderwal@afnic.fr>
Reuse code from Zonemaster::Engine for IP validation

Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>
lib/Zonemaster/CLI.pm Outdated Show resolved Hide resolved
@matsduf matsduf dismissed their stale review August 28, 2024 13:31

Looks fine now.

Better hint for --ds option

Co-authored-by: Mats Dufberg <mats.dufberg@iis.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V-Minor Versioning: The change gives an update of minor in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect header placement when using options '--ns' or '--ds'
4 participants