Debugging and logging improvements #476
Conversation
bin/test_sequencer
Outdated
| @@ -57,7 +57,7 @@ if [[ $i -eq $WAITING ]]; then | |||
| false | |||
| fi | |||
|
|
|||
| bin/sequencer -v $site_path $project_id $device_id $serial_no | |||
| bin/sequencer ${SEQUENCER_OPTS} $site_path $project_id $device_id $serial_no | |||
There was a problem hiding this comment.
Could we be consistent about $ { } braces (I prefer the latter)
There was a problem hiding this comment.
I'm not sure what you mean by "latter" as you only mention one, but I made it consistent with the others on the line.
| @@ -599,7 +601,7 @@ protected void updateConfig(String reason) { | |||
| updateConfig(SubFolder.LOCALNET, deviceConfig.localnet); | |||
| updateConfig(SubFolder.BLOBSET, deviceConfig.blobset); | |||
| updateConfig(SubFolder.DISCOVERY, deviceConfig.discovery); | |||
| recordDeviceConfig(reason); | |||
| localConfigChange(reason); | |||
There was a problem hiding this comment.
I can't comment on L 636 here.
I am a little concerned about adding arbitrary fields that can't be validated by the schema. Could we consider putting the nonce field, without the field itself being defined by the schema, into a "debug" map that IS formally defined? We can define the whole thing as hands-off/no expectations.
There was a problem hiding this comment.
The field should never be exposed to a place where it should be validated. It's meant to be low-level under-the-hood, which means that it should be removed before anything "external" to the core sees it. Adding it to the schema in some way implies that it has some semantic meaning that people "outside the core" should care about, which it very much explicitly is not.
The debug section would work, yes -- but -- I don't have much more time to spend on this (other things are backed up), so the options I'd put out there are 1) not for now, maybe later, 2) not submit this and have it linger, 3) you could pick it up and convert to the debug structure. I'm not a big fan of #2, and also consider that for better of worse the config nonce field is already there (it's not new) -- this is just cleaning it up some.
There was a problem hiding this comment.
How about renaming it to debug_nonce? That would achieve more goals and make it stand out more as hands-off.
There was a problem hiding this comment.
Changed to debug_config_nonce
| @@ -599,7 +601,7 @@ protected void updateConfig(String reason) { | |||
| updateConfig(SubFolder.LOCALNET, deviceConfig.localnet); | |||
| updateConfig(SubFolder.BLOBSET, deviceConfig.blobset); | |||
| updateConfig(SubFolder.DISCOVERY, deviceConfig.discovery); | |||
| recordDeviceConfig(reason); | |||
| localConfigChange(reason); | |||
There was a problem hiding this comment.
How about renaming it to debug_nonce? That would achieve more goals and make it stand out more as hands-off.
No description provided.