Create options for human-readable output formats #437

acaird · 2020-05-25T20:03:10Z

Added a -summary and -longsummary options to zlint.

The -summary option produces output that looks like:

+-------+--------------+
| LEVEL | # OCCURANCES |
+-------+--------------+
| info  |            0 |
| warn  |            1 |
| error |            7 |
| fatal |            0 |
+-------+--------------+

and the -longsummary produces output that looks like:

+-------+--------------+------------------------------------------+
| LEVEL | # OCCURANCES |                 DETAILS                  |
+-------+--------------+------------------------------------------+
| info  |            0 |  -                                       |
| warn  |            1 | w_ext_san_critical_with_subject_dn       |
| error |            7 | e_cert_policy_iv_requires_country        |
|       |              | e_ca_key_cert_sign_not_set               |
|       |              | e_ca_organization_name_missing           |
|       |              | e_ca_crl_sign_not_set                    |
|       |              | e_ca_country_name_missing                |
|       |              | e_sub_ca_crl_distribution_points_missing |
|       |              | e_sub_cert_not_is_ca                     |
| fatal |            0 |  -                                       |
+-------+--------------+------------------------------------------+

The only significant changes to zlint was to export statusLabelToLintStatus from result.go and to pull in tablewriter.

Thanks for considering these changes.

Linting the test file `testdata/utf8ControlX88.pem` results in: ``` +-------+--------------+ | LEVEL | # OCCURANCES | +-------+--------------+ | info | 0 | | warn | 7 | | error | 15 | | fatal | 0 | +-------+--------------+ ```

cardonator · 2020-05-25T20:19:41Z

Wow, this is awesome. Seriously. Thanks for contributing that. I have been wanting something like this for a while but haven't had any time to work on it.

zakird

LGTM. Thanks!

zakird · 2020-05-25T20:28:42Z

I've committed additional changes to this PR, so tagging @cpu or @dadrian to take a final pass and merge.

cpu

Hi @acaird,

Thanks for the PR! I only had a few minutes this morning and haven't had a chance to do a very thorough review. I did have a few comments worth sharing up front.

v2/cmd/zlint/main.go

v2/go.mod

acaird · 2020-05-28T01:43:39Z

I'm not entirely convinced that removing the tablewriter dependency is worth it, but the new output looks like:

cat testdata/indivValAllBad.pem | ./zlint -summary
| LEVEL | # OCCURRENCES |
+-------+---------------+
|  info |             0 |
|  warn |             1 |
| error |             7 |
| fatal |             0 |

and

cat testdata/indivValAllBad.pem | ./zlint -longsummary
| LEVEL | # OCCURRENCES |                  DETAILS                  |
+-------+---------------+-------------------------------------------+
|  info |             0 |                                        -  |
|  warn |             1 |        w_ext_san_critical_with_subject_dn |
| error |             7 |                      e_sub_cert_not_is_ca |
|       |               |                e_ca_key_cert_sign_not_set |
|       |               |  e_sub_ca_crl_distribution_points_missing |
|       |               |                     e_ca_crl_sign_not_set |
|       |               |                 e_ca_country_name_missing |
|       |               |         e_cert_policy_iv_requires_country |
|       |               |            e_ca_organization_name_missing |
| fatal |             0 |                                        -  |

acaird · 2020-05-28T01:45:13Z

Tests and some consideration of moving the filtering elsewhere will come later.

Thanks for all the feedback!

cpu

Thanks for the iteration on this @acaird! I really appreciate that you were able to rework the table code without the new dependency.

I left some review feedback in this round with some suggestions to refactor the outputSummary func into smaller pieces. If you can handle that & take a crack at some test coverage I think we'll be in a good place to merge. I won't block this PR on trying to consolidate some of the filtering logic back into the ResultSet type, we track that as a follow-up or a nice to have.

Thanks!

v2/cmd/zlint/main.go

acaird · 2020-06-02T21:34:21Z

@cpu thanks for the feedback, I'll clean it up and write some tests, hopefully I'll get to it all within the next week or so

- switched `-longsummary` option to `-longSummary` to be more consistent with existing options - fixed an embarrassing output bug when two categories had the same number of errors

cpu · 2020-06-25T23:20:07Z

👋 @acaird just wanted to check in on this branch. Do you feel like you'll have a chance to address the last bits of feedback in the next few weeks? If you think you might not have the time I definitely understand and would be happy to adopt this PR to get it across the line. Let me know what you would prefer!

acaird · 2020-06-26T01:36:55Z

👋 @cpu. I haven't given up yet! If I can't get something worthwhile by Tuesday I'll put it up for adoption. Sorry for the delays... :/

cpu · 2020-06-26T21:06:04Z

If I can't get something worthwhile by Tuesday I'll put it up for adoption. Sorry for the delays... :/

Please don't apologize, take your time :-) I just want to make sure my feedback isn't the only thing keeping this from landed in tree. Thanks for the update!

Co-authored-by: tld-update-bot <cpu+tldbot@letsencrypt.org>

acaird · 2020-06-30T00:02:52Z

Writing tests in main is not ideal; I can keep working on it or I can split the functions and their requirements into a file that isn't main and test that. Thoughts?

cpu · 2020-07-02T11:49:00Z

I can split the functions and their requirements into a file that isn't main and test that. Thoughts?

@acaird Splitting would be my pref. but it's lightly held. If you find it easier to keep working through main that's OK with me. I don't want to hold you up :-)

v2/formattedoutput/formattedOutput.go

v2/cmd/zlint/main.go

…ce output

cpu

With three other +1 reviews I don't want to hold this useful addition to zlint up any longer on my request for unit tests. I think there's room for flexibility here, especially given it isn't adding or changing any linting logic so i'll go ahead and stamp + merge.

@acaird If you've already started on tests a follow-up PR to add what you have would be great. Thanks!

cpu · 2020-07-20T21:57:26Z

@zakird Thanks for merging, was just typing out a squash commit message.

zakird · 2020-07-20T21:58:21Z

oh 🤦 sorry about that!

cpu · 2020-07-20T22:18:14Z

No worries :-) Happy it's all merged up 🎉

acaird · 2020-07-23T23:05:40Z

Thanks for merging it. I'm glad it's useful. I'm sorry about the tests, they are still forthcoming.

acaird added 2 commits May 25, 2020 11:26

zakird approved these changes May 25, 2020

View reviewed changes

spelling fix

eef788b

cardonator approved these changes May 26, 2020

View reviewed changes

cpu requested changes May 26, 2020

View reviewed changes

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

v2/go.mod Outdated Show resolved Hide resolved

acaird and others added 3 commits May 27, 2020 21:24

Remove tablewriter dependency and reimplement the good parts

31c7bdc

spelling fix

eefe940

Merge branch 'summary' of github.com:acaird/zlint into summary

ec56f06

Fixed a missed merge :(

b285882

cpu requested changes May 31, 2020

View reviewed changes

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

Merge branch 'master' into summary

fb86585

cpu self-assigned this May 31, 2020

acaird and others added 6 commits June 2, 2020 17:47

switched longsummary to longSummary; fixed output bug

832bc81

- switched `-longsummary` option to `-longSummary` to be more consistent with existing options - fixed an embarrassing output bug when two categories had the same number of errors

Cleaned up typos, variable names, formatting

b3c3f69

Merge branch 'summary' of github.com:acaird/zlint into summary

3a60980

Merge branch 'master' into summary

aab1521

Merge branch 'master' into summary

1e6d785

Merge branch 'master' into summary

4be536f

acaird and others added 3 commits June 28, 2020 15:32

autopull: 2020-05-27T14:34:02Z (zmap#441)

83d5bfe

Co-authored-by: tld-update-bot <cpu+tldbot@letsencrypt.org>

gTLD autopull: 2020-05-28T14:35:00Z (zmap#442)

307f3e8

Co-authored-by: tld-update-bot <cpu+tldbot@letsencrypt.org>

acaird added 2 commits June 28, 2020 15:33

Moved structure creation out of function into a method for reporting

3aaea14

Merge branch 'summary' of github.com:acaird/zlint into summary

e67c57f

cpu and others added 3 commits July 2, 2020 07:49

Merge branch 'master' into summary

0a21f09

Moved the formatted output routines out of main

e12bda4

Merge branch 'summary' of github.com:acaird/zlint into summary

28441a5

dadrian approved these changes Jul 6, 2020

View reviewed changes

v2/formattedoutput/formattedOutput.go Outdated Show resolved Hide resolved

v2/formattedoutput/formattedOutput.go Outdated Show resolved Hide resolved

cpu reviewed Jul 7, 2020

View reviewed changes

v2/cmd/zlint/main.go Outdated Show resolved Hide resolved

acaird added 4 commits July 7, 2020 21:48

Changed newRT to a pointer receiver

d2079c5

Changed output options to all them all; newlines for nice output

9798f3f

Changed output options to allow printing of them all; newlines for ni…

c21f46c

…ce output

Merge branch 'summary' of github.com:acaird/zlint into summary

8a79935

cpu approved these changes Jul 20, 2020

View reviewed changes

Merge branch 'master' into summary

d436af9

zakird merged commit ca9532d into zmap:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create options for human-readable output formats #437

Create options for human-readable output formats #437

acaird commented May 25, 2020

cardonator commented May 25, 2020

zakird left a comment

zakird commented May 25, 2020

cpu left a comment

acaird commented May 28, 2020

acaird commented May 28, 2020

cpu left a comment

acaird commented Jun 2, 2020

cpu commented Jun 25, 2020

acaird commented Jun 26, 2020

cpu commented Jun 26, 2020

acaird commented Jun 30, 2020

cpu commented Jul 2, 2020

cpu left a comment •

edited

Loading

cpu commented Jul 20, 2020

zakird commented Jul 20, 2020

cpu commented Jul 20, 2020

acaird commented Jul 23, 2020

Create options for human-readable output formats #437

Create options for human-readable output formats #437

Conversation

acaird commented May 25, 2020

cardonator commented May 25, 2020

zakird left a comment

Choose a reason for hiding this comment

zakird commented May 25, 2020

cpu left a comment

Choose a reason for hiding this comment

acaird commented May 28, 2020

acaird commented May 28, 2020

cpu left a comment

Choose a reason for hiding this comment

acaird commented Jun 2, 2020

cpu commented Jun 25, 2020

acaird commented Jun 26, 2020

cpu commented Jun 26, 2020

acaird commented Jun 30, 2020

cpu commented Jul 2, 2020

cpu left a comment • edited Loading

Choose a reason for hiding this comment

cpu commented Jul 20, 2020

zakird commented Jul 20, 2020

cpu commented Jul 20, 2020

acaird commented Jul 23, 2020

cpu left a comment •

edited

Loading