search

voxpupuli / metadata-json-lint

Tool to check the validity of Puppet metadata.json files
Apache License 2.0
29 stars 27 forks source link

Add new --format option #60

Closed whopper closed 7 years ago

whopper commented 7 years ago

These commits add a --format (-f) option to control the output format of errors and warnings. Valid values for --format are 'text' (the default) and 'json'. The 'text' format is similar to the previous format but slightly more structured. 'json' format is brand new.

Tests have been updated to match against the slight changes to 'text' output format as well as resolving some minor inconsistencies in which checks would result in a non-zero exit status.

Due to the more consistent behavior around exit status, these changes are not strictly backwards compatible.

(Submitting on behalf of Jesse, who did all of the legwork on this!)

whopper commented 7 years ago

A little context on this: we're going to be including metadata-json-lint in our SDK and we want each of the tools involved to be able to provide output in both text and json. This will help us elegantly wire everything together in our SDK CLI wrapper and write sensible, standardized reports.

ghoneycutt commented 7 years ago

I would like to learn more about this SDK. Do you have any links you can share?

bastelfreak commented 7 years ago

@dev_el_ops this is probably the SDK you mentioned on cfgmgmtcamp? Do you have any information to share?

rnelson0 commented 7 years ago

@whopper This is nice! This fixes a few other issues incidentally, a good benefit.

As this changes the output formats a lot, we may run afoul of people's tooling, much like our tests, that expects certain string formatting. I think this may need a major version bump to allow people to adjust. However, you can also suggest that the text output isn't an API promise. What do others think?

(The SDK questions should probably move to the puppet-dev or voxpupuli mailing lists or this PR will get crowded fast)

scotje commented 7 years ago

Yeah, I think a major version bump would be reasonable. I do think that going forward, the output for --format=text should not be considered an API promise, but since it was the only output available until now, people are likely to have built up expectations around it. Another reason for the version bump would be that the exit status has changed in a couple of scenarios.

(There will be more official SDK information "soon", but in the meantime you can ping "turbodog" on the Puppet Community Slack if you want to talk about it.)

ghoneycutt commented 7 years ago

I doubt that people have expectations around the output as I've never seen any work flow or tool that does that. m-j-l is most often invoked now as part of rake validate due to it being included in puppetlabs_spec_helper. People rely on the exit code, not the formatting. I don't think we need a major version bump because the output of the text changed when there is a failure.

Once moving to using structured formatting with json, if that changes, then we should look at major version bumps for the changes, because then we are making a promise on the output.

scotje commented 7 years ago

@ghoneycutt This refactor also exposed a couple inconsistencies in the exit code that was returned. (I would have to dig back in to remember the exact details, but I think basically there were times when warnings and --fail-on-warnings weren't interacting as expected.)

scotje commented 7 years ago

I think the cases where the exit-code changed are the three tests where we had to add --no-fail-on-warnings to make them pass.

rnelson0 commented 7 years ago

@scotje that's #53, and when researching that there were some other cases I identified - part of why I like this change so much :)

rnelson0 commented 7 years ago

@ghoneycutt since this will change some exit codes in various cases, do you think that raises this to a MAJOR bump or is it still a MINOR?

scotje commented 7 years ago

Hi folks, just wanted to check in on this. Since this PR doesn't include any version changes or other release prep, maybe this can be merged before the versioning question is resolved?