Respect desired logLevel

[marstr]

Despite setting the consoleLogLevel to 'error' here:
https://github.com/Azure/azure-rest-api-specs/blob/32610853cf560d7454b88ec4ca79a109e8525851/test/semantic.js#L19

A lot of extra lines are being output in CI right now, as seen here:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/351021743#L694

[marstr]

@amarzavery, @olydis please take a look :)

It seems like I'm not a "Contributor" here, so I can't straight up request a review from you in GitHub UI.

[olydis]

I don't know this code at all, but the fact that it was guarded by log.consoleLogLevel === 'json' makes me fear that automation may try to parse the output of this script as JSON. Or will this message not appear on stdout for some reasons I'm unaware of? (like the arg you added in package.json)

[marstr]

The way I read this block, it was overwriting the desired consoleLogLevel in any case except for json and off. By removing the check, it should never overwrite the specified consoleLogLevel, including json and off. I may be dumb though, let me know if I've messed up mentally applying DeMorgans law :)

[marstr]

Errr, I guess I'm making the assumption that whatever is converting stdout to JSON can handle a message it wouldn't have seen before. I'm also assuming that consoleLogLevel off knows to totally disregard things printed to info. I guess the safest way to handle this would be to just keep that check there, with the chance that it adds a little redundancy.

I'll add the check back to be safe.

[amarzavery]

Yes, in cases where someone set the log level to error then they do not see anything on the command prompt when running oav manually. Folks said they did not know if everything was ok. Hence I did the above thing to explicitly say that "No errors were found." 👍 .

[amarzavery]

Error is above info in the log level severity...

[marstr]

Ah, thanks for the context, @amarzavery. With my PR in the specs repository to use the "min" mocha reporter, it should clear that up without needing to print the "No errors were found." message.

[amarzavery]

But your usecase is genuine. While running this as a script you don't need that info statement.
We can have a variable named mode that can tell us how oav is being used. If the mode is cli then we can display the log.info(..) statement. If it is script then we can choose to not log that statement.

If you are writing a script then the entry point is via the file indicated in "main" property of package.json. For cli it is the bin property https://github.com/Azure/oav/blob/master/package.json#L49-L52

[amarzavery]

You can export a variable here in lib/validate.js called exports.isCliMode = true; (We assume that folks would be running oav as a CLI).
Then in index.js over here validate.isCliMode = false;.

if (validator.specValidationResult.validityStatus) {
    if (!(log.consoleLogLevel === 'json' || log.consoleLogLevel === 'off') && exports.isCliMode) {
      let consoleLevel = log.consoleLogLevel;
      log.consoleLogLevel = 'info';
      log.info('No Errors were found.');
      log.consoleLogLevel = consoleLevel;
    }
}

[marstr]

Hmmm, that seems counter-intuitive to me. Maybe a simpler option would be to have the default consoleLogLevel set to 'info'? In my mind anyway, I'd rather have the tool behave consistently regardless of how it was invoked.

[amarzavery]

I see. That is a valid point too.

[amarzavery]

Can you also update the version of moment to ^2.21.0 in package.json (the 2.18.1 version has a security vulnerability that was fixed in the newer version).

[marstr]

You make me a very happy man, @amarzavery.

[amarzavery]

@marstr - One more thing. Can you please delete the package-lock.json and also delete the node_modules folder and excute npm install again. This should generate a new lock file. Can you please send a PR with that new lock file. The current one still points to 2.18.1 version of moment.