Arvados

Updates at b701b0f38 - branch 18487-vocab-config-check
Test run: developer-run-tests: #2882

• Adds vocabulary checking to arvados-server config-check, ignoring the special case when there's an API.VocabularyPath value set, but no file is found, to avoid false positives on non-controller nodes.
• Improves error detection by also checking for duplicated JSON keys that were previously ignored.
• Improves error reporting by collecting all the issues before returning, so that the config-check command and also the /_health/vocabulary endpoint do a full issue report and not annoy the site admin by providing errors one at a time.
• Improves the Metadata vocabulary documentation page on the Admin section.
• Adds tests.

Lucas Di Pentima wrote:

Updates at b701b0f38 - branch 18487-vocab-config-check
Test run: developer-run-tests: #2882

• Adds vocabulary checking to arvados-server config-check, ignoring the special case when there's an API.VocabularyPath value set, but no file is found, to avoid false positives on non-controller nodes.
• Improves error detection by also checking for duplicated JSON keys that were previously ignored.
• Improves error reporting by collecting all the issues before returning, so that the config-check command and also the /_health/vocabulary endpoint do a full issue report and not annoy the site admin by providing errors one at a time.
• Improves the Metadata vocabulary documentation page on the Admin section.
• Adds tests.

In lib/config/cmd.go:

The addition to config-check isn't ideal; if there are any warnings from the checks above it (e.g. if ManagementToken is not set), config-check aborts without even doing the new test, and nothing is printed.

Would it be possible to move the new tests above the diff command, and send their output to logbuf rather than returning immediately so they are handled the same way as other warnings? That would also make the output more uniform, for better or worse we output structured logs from config-check, which is hard to read, but at least it will be consistent.

Otherwise, LGTM, thanks.

Updates at 4f1341cf0
Test run: developer-run-tests: #2888

Thanks for the suggestions, I totally missed the logging part.
I've moved the vocabulary checking up a little and use the logger to output the reporting (sadly, it's kind of unreadable now) so that it can be handled the same way as the rest.
Had to adjust a test regexp, too.

Lucas Di Pentima wrote:

Updates at 4f1341cf0
Test run: developer-run-tests: #2888

Thanks for the suggestions, I totally missed the logging part.
I've moved the vocabulary checking up a little and use the logger to output the reporting (sadly, it's kind of unreadable now) so that it can be handled the same way as the rest.
Had to adjust a test regexp, too.

LGTM, thanks! I think we need to change the default output for config-check to human readable, perhaps with a machine-friendly output format as an optional flag. But not as part of this ticket.