Add FileReporter in addition to existing ConsoleReporter

[bbrouwer]

Add an additional Reporter that I can enable with a -Dlqlint.report.txt=report.txt parameter.

This looks relatively straightforward as the Reporters is already defined as a Collection. If the system property is undefined, the reporter will do nothing.

This will allow me to more easily extract the rule violations from a CI build and, for example, add them as a comment to a pull request.

Alternatively, I considered using the Java Service Provider Interface to accomplish this. This would allow other custom reporters to be contributed by others by simply adding them to the classpath.

[bbrouwer]

Perhaps a Markdown version would be useful too. I know I would use it: -Dlqlint.report.markdown=report.md

[davidjgoss]

I know that @luke-rogers was starting to build a JunitReporter under https://github.com/whiteclarkegroup/liquibase-linter/compare/junit-reporter, and it looks like that change is building up the kind of framework you're talking about. I think something similar to the rules where it kind of self-registers with a name via SPI and then as a user we choose which reporters to activate using system properties and/or something in the config file. What do you think?

I like the idea of having a plain text reporter but it being markdown. I could see the basic built-in formatters being:

• console
• junit
• json
• markdown

Where the output from either of the last two could be piped to something else to produce a richer report.

[bbrouwer]

I like what I see in that branch. It looks like no work has been done on it in quite some time. I will take some of the ideas from there when building this myself. I hope to have a PR for this next week.

I'm thinking we can by default only enable the ConsoleReporter. I'm thinking of adding a system parameter so we could call maven with: -Dlqlint.reports=ConsoleReporter,TextReporter,MarkdownReporter and so forth (using the class simple name to filter)

I see he added in the notion of a PASSED report item. That makes sense, too. Should there be another parameter: -Dlqlint.report.types=ERROR,IGNORED (the default) that allows users to specify what ends up in the report?

Do we also want -Dlqlint.reports.path=target (the default) so I can control where the report is written, such as:

• ./target/report.txt
• ./target/report.md
• ./target/report.xml (for junit someday. I'm not planning on doing this work)
• ./target/report.json (for some day, not my PR)

[davidjgoss]

Yeah I think some flexibility is needed for which reporters are on and where the output goes. For example Cucumber has a nice affordance for this by having a repeatable CLI option so you can do like:

cucumber-js --format json:target/test-report.json --format html:target/documentation.html

BTW I think a Markdown reporter removes the need for a separate txt one, right? Since Markdown by definition is human readable plain text in itself.

[bbrouwer]

Possibly. I know that Markdown for tables can get ugly fast.

What if I wanted to just have plain text that I send in an email? Then I wouldn't really want markdown. It would be more ideal to use HTML for email, but what if I'm a lazy programmer and don't want to figure out how to send html email? I have things broken out so there isn't any significant code duplication.

Here are some markdown examples I'm considering. The first one is definitely harder to implement.

path/to/changelog.xml

Change Set	Status	Rule	Message
first	IGNORED	some-rule	violation message
		next-rule	next message
	ERROR	third-rule	third message
		fourth-rule	fourth message
second	IGNORED	some-rule	violation message
		next-rule	next message
	ERROR	third-rule	third message
		fourth-rule	fourth message

Or alternatively keep it very close to the existing Console logger:

path/to/changelog.xml

Change Set first

IGNORED

• some-rule: violation message
• next-rule: next message

ERROR

• third-rule: third message
• fourth-rule: fourth message

Change Set second

IGNORED

• some-rule: violation message
• next-rule: next message

ERROR

• third-rule: third message
• fourth-rule: fourth message

[bbrouwer]

I do like the cucumber format idea.

[luke-rogers]

Some good ideas here.

Would be good to follow SPI style pattern so we can support 'bring your own reporter'.

I am more in favour of having the config for this in lqlint.json as opposed to -D properties. I like the idea of just having a single place to look for the liquibase linter config where possible. I suppose this comes down to wether we think people are going to want to do separate runs with different reporter outputs, using the same lqlint.json. I would have thought most users would have a fixed set of reports they would want as output on each run. Maybe there is an argument for something like -Dlqlint.reports.path=target where you might want the reports outputted in a different location based on where its being run, e.g. local or ci?.

If this config is specified in lqlint.json then it is easy to share between projects as lqlint.json can be loaded from a jar. Also it would allow us to extend to support more complex configurations like having different report items for different reporters, e.g. some may only way ERROR or IGNORED in console, but all in markdown file.

What do you two think?

[davidjgoss]

@bbrouwer I really like your table-based idea for a markdown report!

What if I wanted to just have plain text that I send in an email? Then I wouldn't really want markdown.

Why not? The beauty of Markdown for me has always been that if you do zero processing you still have nice readable plain text. I do take your point about tables - that's maybe the one exception where it can look a bit clunky in raw form, but I think that's not a problem if you get the spacing right. Looks like there are libraries out there (e.g. https://github.com/Steppschuh/Java-Markdown-Generator) for doing just that. Ideally we wouldn't increase our dependency footprint, but could borrow from there.

@luke-rogers yes I tend to agree. Even with cucumber the formatter options generally end up in the config file rather than the CLI. So something like:

{
  "reporting": {
    "json": "target/lqlint.json",
    "markdown": "target/lqlint.md"
  },
  "rules": {
    ...
  }
}

(and console reporter is implicitly always on)

[bbrouwer]

I like the idea of putting the reporting section in the lqlint.json file, but I'm not sure it takes care of specifically what I'm trying to do. My goal is to have a Jenkins pipeline library that invokes Maven to produce these reports and display the results in various locations, such as PR comments. I'd rather not rely upon a developer configuring the reporting section correctly in the repository.

The -D parameters do accomplish my goal as the Jenkins pipeline library will be in control of adding them to the Maven command.

If we coupled this with the import section from #107, then I could import what I consider a standard reporting configuration by importing both the rules and the reporting sections. I would say any definition of a reporting section overrides all imported reporting sections and doesn't try to merge the two together. In the case of multiple imported reporting sections, would we theoretically produce two markdown reports if both included a reporting section for markdown? That is sort of my plan for the rules in #107.

If we allow the import of the reporting section, I think then I would be comfortable with dropping the -D parameters.

[bbrouwer]

Regarding the idea of being able to configure each reporter individually (console showing ERROR and IGNORE, others showing more), I could see that working. I might argue that is deserving another issue so I can scope this to something more easily attainable in the short term. But the flexible parsing of the lqlint.json could work for reporting as well, allowing in addition to what @davidjgoss outlined:

{
  "reporting": {
    "json": {
      "path": "target/lqlint.json",
      "filter": [ "ERROR", "IGNORE", "PASSED" ]
    }
  }
}