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

Related to zonemaster-engine issue 578 #157

Merged

Conversation

vlevigneron
Copy link
Contributor

DO NOT MERGE NOW, do a test first to check if it is what you are waiting for

@vlevigneron vlevigneron changed the base branch from master to develop July 23, 2020 19:03
@vlevigneron vlevigneron requested a review from matsduf July 27, 2020 10:25
@matsduf matsduf changed the title Related to zonemaster issue 578 Related to zonemaster-engine issue 578 Jul 27, 2020
@matsduf
Copy link
Contributor

matsduf commented Jul 27, 2020

@vlevigneron, does this PR depend on the code change by zonemaster/zonemaster-engine/pull/762?

@matsduf matsduf added the A-Code label Jul 27, 2020
@matsduf matsduf added this to the v2020.1 milestone Jul 27, 2020
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.

I have tested this together with zonemaster/zonemaster-engine#762.

The way it is presented in "normal" view and --jason is fine. If --raw is selcted then --show_testcase gives both test case and module:

# zonemaster-cli zonemaster.net --no-ipv6 --level INFO --show_testcase --raw
   0.00 INFO      SYSTEM:UNSPECIFIED:GLOBAL_VERSION version=v3.1.2
   0.33 INFO      BASIC:BASIC01:HAS_PARENT pname=net; zone=zonemaster.net
   1.57 INFO      BASIC:BASIC02:IPV6_DISABLED address=2001:67c:124c:100a::45; ns=ns.nic.se; rrtype=NS
   1.57 INFO      BASIC:BASIC02:IPV4_ENABLED address=91.226.36.45; ns=ns.nic.se; rrtype=NS
   1.58 INFO      BASIC:BASIC02:HAS_NAMESERVERS address=91.226.36.45; ns=ns.nic.se; nsnlist=ns.nic.se.,ns2.nic.fr.,ns3.nic.se.
   1.58 INFO      BASIC:BASIC02:IPV4_ENABLED address=192.93.0.4; ns=ns2.nic.fr; rrtype=NS
   1.63 INFO      BASIC:BASIC02:HAS_NAMESERVERS address=192.93.0.4; ns=ns2.nic.fr; nsnlist=ns.nic.se.,ns2.nic.fr.,ns3.nic.se.
(...)

Since there is already a show_module options that is unneeded. It is better if --show_testcase just shows the test case. If you want the module too, then you use both options.

# zonemaster-cli --show_module zonemaster.net --no-ipv6 --level INFO --show_testcase --raw
   0.00 INFO      SYSTEM       SYSTEM:UNSPECIFIED:GLOBAL_VERSION version=v3.1.2
   0.28 INFO      BASIC        BASIC:BASIC01:HAS_PARENT pname=net; zone=zonemaster.net
   1.54 INFO      BASIC        BASIC:BASIC02:IPV6_DISABLED address=2001:67c:124c:100a::45; ns=ns.nic.se; rrtype=NS
   1.54 INFO      BASIC        BASIC:BASIC02:IPV4_ENABLED address=91.226.36.45; ns=ns.nic.se; rrtype=NS
   1.55 INFO      BASIC        BASIC:BASIC02:HAS_NAMESERVERS address=91.226.36.45; ns=ns.nic.se; nsnlist=ns.nic.se.,ns2.nic.fr.,ns3.nic.se.
(...)

Secondly, it will be easier to read if the test case is presented as the module, i.e. with space before the message tag. E.g.:

   0.00 INFO      SYSTEM       UNSPECIFIED  GLOBAL_VERSION version=v3.1.2
   0.28 INFO      BASIC        BASIC01      HAS_PARENT pname=net; zone=zonemaster.net
   1.54 INFO      BASIC        BASIC02      IPV6_DISABLED address=2001:67c:124c:100a::45; ns=ns.nic.se; rrtype=NS
   1.54 INFO      BASIC        BASIC02      IPV4_ENABLED address=91.226.36.45; ns=ns.nic.se; rrtype=NS
   1.55 INFO      BASIC        BASIC02      HAS_NAMESERVERS address=91.226.36.45; ns=ns.nic.se; nsnlist=ns.nic.se.,ns2.nic.fr.,ns3.nic.se.
(...)

Thirdly, the new option is available with you run zonemaster-cli -h, but not when you run man zonemaster-cli.

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 Sep 28, 2020

@vlevigneron, can this be completed?

@vlevigneron
Copy link
Contributor Author

@matsduf @mattias-p Please review again and check if output is what you are waiting for.

@matsduf
Copy link
Contributor

matsduf commented Oct 12, 2020

@mattias-p, can you review again?

@vlevigneron vlevigneron merged commit 9823b13 into zonemaster:develop Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend messages with test case
3 participants