ROX-27396: Collector configloader should not need to be modified when the protobuf is #1993
Conversation
| sensor::CollectorConfig runtimeConfig; | ||
| google::protobuf::util::JsonParseOptions parseOptions; | ||
| parseOptions.ignore_unknown_fields = true; | ||
| auto status = google::protobuf::util::JsonStringToMessage(jsonStr, &runtimeConfig, parseOptions); |
There was a problem hiding this comment.
It seems silly to first convert to json, then json string, and then to the protobuf class, instead of converting more directly, and it likely is silly. The reason I did this is that this way I don't have to worry as much about the types. E.g. when converting from yaml, in this case, the code assumes that all the values are strings. This is later fixed by JsonStringToMessage. However, if we tried to convert to the protobuf class directly to the json class used by protobuf, we would first have to figure out the type before doing the conversion.
| @@ -42,34 +42,16 @@ TEST(CollectorConfigTest, TestYamlConfigToConfigMultiple) { | |||
| } | |||
There was a problem hiding this comment.
Ideally this would have the same behavior as before and no changes to the unit tests would have to be made. However, there the old code had checks that the networking field was present and not blank and with the new more general approach this check is not done. Also the old allowed for the yaml to have invalid values, but JsonStringToMessage does not allow that.
…name for all test runs
JoukoVirtanen
force-pushed
the
jv-ROX-27396-collector-configloader-should-not-need-to-be-modified-when-the-protobuf-is
branch
from
3a0162e to
f1c0f05
Compare
Description
Currently every time that CollectorConfig protobuf definition is changed, we have to change the code for the ConfigLoader. The changes here make it so that is not necessary.
There are also changes here that make it so that collector logs are written to the correct location. Currently for tests that have sub-tests, collector logs are only written to the directory for the first sub-test. This PR fixes that.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Unit and integration tests are sufficient.