zlint: refactor lint reg., allow filtering lints used. #372
Conversation
This replaces the exported `lint.LintMap` field (`map[string]*Lint`) that was used by `RegisterLint` with a more robust solution based around a `Registry` interface. This allows ZLint users to include/exclude lints by name or source. Top Level API ------------- The `lint.RegisterLint` function remains the same, meaning individual lints are not changed. Lints that call this function are added to the global registry. Clients can access this registry with `lint.GlobalRegistry()`. Similarly the top level `zlint.LintCertificate` remains unchanged. It lints the provided cert with all lints in the global registry. The old `zlint.LintCertificateFiltered` function that accepted a lint name regex to filter the lints applied is replaced by a new function `zlint.LintCertificateEx` that allows specifying a `lint.Registry` explicitly. The `lint.Source` type was changed from an int enum to a string enum. This makes it easier to work with as a consumer (e.g. via command line flags, and JSON output) and since the number of lints (and sources) is small the benefits to using an int enum type are minimal. The serialized form of Lints now includes the `Source` field in the output as `"source"`. Registry -------- The `Registry` interface also allows finding all lint names with `Names()`, finding all lint sources with `Sources()`, finding a specific lint by name with `ByName()`, and finding all lints for a given source with `BySource()`. The `zlint.EncodeLintDescriptionsToJSON` function is now implemented by the `Registry` interface as `WriteJSON`. This makes it easier to encode a subset of the Registry's lints by filtering the global registry. Like before (with the exported `map[string]*Lint`) the registry is not safe for concurrent updates. That's fine for the current ZLint codebase but is something we may want to consider addressing in the future. Registry Filtering ------------------ Filtering of lints to be run is now done with the `lint.Registry.Filter` function and corresponding `lint.FilterOptions` type. This allows filtering a registry to include/exclude lints by name (or using a name regex), and to include/exclude lints by source. By filtering the global registry and then providing it explicitly to `zlint.LintCertificateEx` callers have control over exactly what lints will be applied. Filtering operations are applied with the following precedence: excludes by source > includes by source > excludes by name > includes by name. E.g. excluding a source and then trying to include a lint in that excluded source by name will not work. The source exclusion happens first. ZLint CMD Updates ----------------- The `zlint` command (`cmd/zlint/main.go`) is updated to add three new command line flags: 1. `-list-lints-sources` - Prints a list of lint sources, one per line. 2. `-excludeSources` - Comma-separated list of lint sources to exclude. 3. `-includeSources` - Comma-separated list of lint sources to include. 4. `-nameFilter` - Regex used to match lint names to include (cannot be used at the same time as `-excludeSources` or `-includeSources) Two existing flags are renamed: 1. `-include` becomes `-includeNames` 2. `-exclude` becomes `-excludeNames`. Notably all three list flags (`-list-lints-json`, `-list-lints-schema` and `-list-lints-sources`) now operate **after** applying the include/exclude filters, allowing an easy way to find which lints/sources will be run with the filtered command line flags in use. Integration Test Updates ------------------------ Matching the `zlint` command the integration test (`integration/integration_test.go`) command line flags are updated to allow including/excluding lints by source.
This makes the default registry safe for concurrent access. ZLint does not currently register lints from multiple go routines/threads but we may as well make it safe for the future.
|
Based on some out of band feedback I'm going to try and restructure the |
|
Thanks for all your hard work on this, @cpu. It's looking really good so far. With the large amount of changes in this PR, would it also be possible to add a flag that tells LintCertificate to only return lints that met some condition? I.E. in my case, I only care about lints that had a status that wasn't NA or Pass. It would be nice if LintCertificate could be instructed to only return those lints and not every lint that ran. This would be particularly helpful when using the CLI as the output of the tool is pretty hard to parse within a terminal. |
Thanks for taking a look!
@cardonator That seems like a reasonable request. I had to implement this sort of filtering in the CFSSL ZLint integration: https://github.com/cloudflare/cfssl/blob/644917271238216c94866f021e2b24ce54555848/signer/local/local.go#L183-L185 I'll see about rolling that in to this branch (or will file a follow-up issue if I don't get to it). |
I spent some time fiddling with this (WIP/broken code here: https://github.com/cpu/zlint/tree/cpu-refactors-lint-registration-again). My experience is that the idea doesn't play nicely with the way lint registration is handled, and the split between the I'm still happy with the design in this PR w/ the |
|
Overall, I think this is really good @cpu. If we can clear up CI, I'm happy to merge. |
The ZSchema functionality isn't required anymore.
@dadrian I think the CI failure was unrelated to the code. Yesterday I accidentally pushed my branch to the It looks like CI is happy again as of d4cbcd6. |
One other note: can I request this gets squash merged with the PR desc as the commit message? I think that will be the cleanest. Alternatively I can rebase and squash to one commit before merging but I think it's easier to let Github do the work. |
@cardonator Unfortunately I had to punt lint result filtering to a separate issue for now: #384 I'll try to come back around at that after a RC and reviewing the other open PRs. |
|
It's all good, @cpu! Thanks for all your work on these large refactorings! |
This replaces the exported
lint.LintMapfield (map[string]*Lint) that was used byRegisterLintwith a more robust solution based around aRegistryinterface. This allows ZLint users to include/exclude lints by name or source.Top Level API
The
lint.RegisterLintfunction remains the same, meaning individual lints are not changed. Lints that call this function are added to the global registry. Clients can access this registry withlint.GlobalRegistry(). Similarly the top levelzlint.LintCertificateremains unchanged. It lints the provided cert with all lints in the global registry.The old
zlint.LintCertificateFilteredfunction that accepted a lint name regex to filter the lints applied is replaced by a new functionzlint.LintCertificateExthat allows specifying alint.Registryexplicitly. The same regex filtering can be done by pre-filtering the provided registry (See notes below).The
lint.Sourcetype was changed from an int enum to a string enum. This makes it easier to work with as a consumer (e.g. via command line flags, and JSON output) and since the number of lints (and sources) is small the benefits to using an int enum type are minimal. The serialized form of Lints now includes theSourcefield in the output as"source".Registry
The
Registryinterface also allows finding all lint names withNames(), finding all lint sources withSources(), finding a specific lint by name withByName(), and finding all lints for a given sourcewith
BySource().The
zlint.EncodeLintDescriptionsToJSONfunction is now implemented by theRegistryinterface asWriteJSON. This makes it easier to encode a subset of the Registry's lints by filtering the global registry.Like before (with the exportedEdit: I decided it made sense to add locking to future proof the implementation for thread safety, see 6072e24 The implementation in this branch is now safe for concurrent access/registrationmap[string]*Lint) the registry is not safe for concurrent updates. That's fine for the current ZLint codebase but is something we may want to consider addressing in the future.Registry Filtering
Filtering of lints to be run is now done with the
lint.Registry.Filterfunction and correspondinglint.FilterOptionstype. This allows filtering a registry to include/exclude lints by name (or using a name regex), and to include/exclude lints by source.By filtering the global registry and then providing it explicitly to
zlint.LintCertificateExcallers have control over exactly what lints will be applied.Filtering operations are applied with the following precedence: excludes by source > includes by source > excludes by name > includes by name.
E.g. excluding a source and then trying to include a lint in that excluded source by name will not work. The source exclusion happens first.
ZLint CMD Updates
The
zlintcommand (cmd/zlint/main.go) is updated to add four new command line flags:-list-lints-sources- Prints a list of lint sources, one per line.-excludeSources- Comma-separated list of lint sources to exclude.-includeSources- Comma-separated list of lint sources to include.-nameFilter- Regex used to match lint names to include (cannot be used at the same time as-excludeSourcesor `-includeSources)Two existing flags are renamed:
-includebecomes-includeNames-excludebecomes-excludeNames.Notably all three list flags (
-list-lints-json,-list-lints-schemaand-list-lints-sources) now operate after applying the include/exclude filters, allowing an easy way to find which lints/sources will be run with the filtered command line flags in use.Integration Test Updates
Matching the
zlintcommand the integration test (integration/integration_test.go) command line flags are updated to allow including/excluding lints by source.Resolves #344