Plugin error should include output

[milton0825]

When there is an error when executing the plugin, include the stdin and stdout from output and log it

[nilslice]

@milton0825 thanks for catching this -- makes sense to me at first glance.. will go through some local testing after merging this into the pr branch & ping you if any questions pop up!

[nilslice]

@milton0825 - after running this against our test suite, the CombinedOutput returns, for example:

plugin-sample-error: some error (/go/bin/plugin-sample-error) {"current":{"definitions":[{"protopath":"testdata:/:getProtoFiles:/:exclude:/:test.proto","def":{"messages":[{"name":"Test","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:exclude.proto","def":{"messages":[{"name":"Exclude","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:include:/:exclude.proto","def":{"messages":[{"name":"Exclude","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:include:/:include.proto","def":{"messages":[{"name":"Include","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"include"}}},{"protopath":"testdata:/:imports_options.proto","def":{"enums":[{"name":"TestEnumOption","enum_fields":[{"name":"FIRST"},{"name":"SECOND","integer":1},{"name":"SEGUNDO","integer":3,"options":[{"name":"(my_enum_value_option)","value":"321"}]}],"reserved_ids":[2]}],"messages":[{"name":"Channel","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string"},{"id":3,"name":"description","type":"string"}],"options":[{"name":"(ext.persisted)","aggregated":[{"name":"opt1","value":"true"},{"name":"opt2","value":"false"}]}]},{"name":"Channel2","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string","options":[{"name":"(personal)","value":"true"},{"name":"(owner)","value":"test"}]},{"id":3,"name":"description","type":"string","options":[{"name":"(custom_options_commas)","aggregated":[{"name":"personal","value":"true"},{"name":"internal","value":"false"},{"name":"owner","value":"some owner"}]}]},{"id":5,"name":"address","type":"string","options":[{"name":"(custom_options)","aggregated":[{"name":"personal","value":"true"},{"name":"internal","value":"false"},{"name":"owner","value":"some owner"}]}]}],"maps":[{"key_type":"string","field":{"id":4,"name":"map","type":"int32","options":[{"name":"(personal)","value":"true"}]}}],"options":[{"name":"(ext.persisted)","value":"true"}]},{"name":"FieldOptions","fields":[{"id":1,"name":"personal","type":"bool"},{"id":2,"name":"internal","type":"bool"},{"id":3,"name":"owner","type":"string"}]},{"name":"google.protobuf.FieldOptions","fields":[{"id":50000,"name":"custom_options","type":"FieldOptions"}]}],"imports":[{"path":"google/protobuf/descriptor.proto"},{"path":"testdata/test.proto"}],"package":{"name":"test"}}},{"protopath":"testdata:/:test.proto","def":{"enums":[{"name":"TestEnum","enum_fields":[{"name":"FIRST"},{"name":"SECOND","integer":1},{"name":"SEGUNDO","integer":1}],"reserved_ids":[2]},{"name":"ContainsEnum.NestedEnum","enum_fields":[{"name":"ABC","integer":1},{"name":"DEF","integer":2}],"reserved_ids":[101],"reserved_names":["DEPTH"]}],"messages":[{"name":"TestRequest"},{"name":"TestResponse"},{"name":"Channel","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string"},{"id":3,"name":"description","type":"string"},{"id":4,"name":"foo","type":"string"},{"id":5,"name":"age","type":"int32"},{"id":101,"name":"newnew","type":"int32"},{"id":44,"name":"msg","type":"A"}],"reserved_ids":[6,8,9,10,11],"messages":[{"name":"A","fields":[{"id":1,"name":"id","type":"int32"}]}]},{"name":"Display","fields":[{"id":1,"name":"width","type":"int32"},{"id":2,"name":"height","type":"int32"},{"id":44,"name":"msg","type":"A"}],"maps":[{"key_type":"string","field":{"id":4,"name":"b_map","type":"int32"}}],"reserved_ids":[3],"reserved_names":["a_map","single_quoted"],"messages":[{"name":"A","fields":[{"id":1,"name":"id","type":"int64"}],"reserved_ids":[2]}]},{"name":"ContainsEnum","fields":[{"id":1,"name":"id","type":"int32"},{"id":2,"name":"value","type":"NestedEnum"}]},{"name":"PreviousRequest","fields":[{"id":4,"name":"name","type":"string"},{"id":9,"name":"is_active","type":"bool"}]}],"services":[{"name":"TestService","rpcs":[{"name":"TestRpc","in_type":"TestRequest","out_type":"TestResponse","options":[{"name":"(test_option)","value":"option_value"},{"name":"(test_option_2)","value":"option_value_3"}]}]},{"name":"ChannelChanger","rpcs":[{"name":"Next","in_type":"NextRequest","out_type":"Channel","in_streamed":true},{"name":"Previous","in_type":"PreviousRequest","out_type":"Channel","out_streamed":true}]}],"package":{"name":"dataset"},"options":[{"name":"java_multiple_files","value":"true"},{"name":"java_package","value":"test.java.package"},{"name":"java_outer_classname","value":"TestClass"}]}}]},"updated":{"definitions":[{"protopath":"testdata:/:getProtoFiles:/:exclude:/:test.proto","def":{"messages":[{"name":"Test","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:exclude.proto","def":{"messages":[{"name":"Exclude","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:include:/:exclude.proto","def":{"messages":[{"name":"Exclude","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"exclude"}}},{"protopath":"testdata:/:getProtoFiles:/:include:/:include.proto","def":{"messages":[{"name":"Include","fields":[{"id":1,"name":"name","type":"string"}]}],"package":{"name":"include"}}},{"protopath":"testdata:/:imports_options.proto","def":{"enums":[{"name":"TestEnumOption","enum_fields":[{"name":"FIRST"},{"name":"SECOND","integer":1},{"name":"SEGUNDO","integer":3,"options":[{"name":"(my_enum_value_option)","value":"321"}]}],"reserved_ids":[2]}],"messages":[{"name":"Channel","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string"},{"id":3,"name":"description","type":"string"}],"options":[{"name":"(ext.persisted)","aggregated":[{"name":"opt1","value":"true"},{"name":"opt2","value":"false"}]}]},{"name":"Channel2","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string","options":[{"name":"(personal)","value":"true"},{"name":"(owner)","value":"test"}]},{"id":3,"name":"description","type":"string","options":[{"name":"(custom_options_commas)","aggregated":[{"name":"personal","value":"true"},{"name":"internal","value":"false"},{"name":"owner","value":"some owner"}]}]},{"id":5,"name":"address","type":"string","options":[{"name":"(custom_options)","aggregated":[{"name":"personal","value":"true"},{"name":"internal","value":"false"},{"name":"owner","value":"some owner"}]}]}],"maps":[{"key_type":"string","field":{"id":4,"name":"map","type":"int32","options":[{"name":"(personal)","value":"true"}]}}],"options":[{"name":"(ext.persisted)","value":"true"}]},{"name":"FieldOptions","fields":[{"id":1,"name":"personal","type":"bool"},{"id":2,"name":"internal","type":"bool"},{"id":3,"name":"owner","type":"string"}]},{"name":"google.protobuf.FieldOptions","fields":[{"id":50000,"name":"custom_options","type":"FieldOptions"}]}],"imports":[{"path":"google/protobuf/descriptor.proto"},{"path":"testdata/test.proto"}],"package":{"name":"test"}}},{"protopath":"testdata:/:test.proto","def":{"enums":[{"name":"TestEnum","enum_fields":[{"name":"FIRST"},{"name":"SECOND","integer":1},{"name":"SEGUNDO","integer":1}],"reserved_ids":[2]},{"name":"ContainsEnum.NestedEnum","enum_fields":[{"name":"ABC","integer":1},{"name":"DEF","integer":2}],"reserved_ids":[101],"reserved_names":["DEPTH"]}],"messages":[{"name":"TestRequest"},{"name":"TestResponse"},{"name":"Channel","fields":[{"id":1,"name":"id","type":"int64"},{"id":2,"name":"name","type":"string"},{"id":3,"name":"description","type":"string"},{"id":4,"name":"foo","type":"string"},{"id":5,"name":"age","type":"int32"},{"id":101,"name":"newnew","type":"int32"},{"id":44,"name":"msg","type":"A"}],"reserved_ids":[6,8,9,10,11],"messages":[{"name":"A","fields":[{"id":1,"name":"id","type":"int32"}]}]},{"name":"Display","fields":[{"id":1,"name":"width","type":"int32"},{"id":2,"name":"height","type":"int32"},{"id":44,"name":"msg","type":"A"}],"maps":[{"key_type":"string","field":{"id":4,"name":"b_map","type":"int32"}}],"reserved_ids":[3],"reserved_names":["a_map","single_quoted"],"messages":[{"name":"A","fields":[{"id":1,"name":"id","type":"int64"}],"reserved_ids":[2]}]},{"name":"ContainsEnum","fields":[{"id":1,"name":"id","type":"int32"},{"id":2,"name":"value","type":"NestedEnum"}]},{"name":"PreviousRequest","fields":[{"id":4,"name":"name","type":"string"},{"id":9,"name":"is_active","type":"bool"}]}],"services":[{"name":"TestService","rpcs":[{"name":"TestRpc","in_type":"TestRequest","out_type":"TestResponse","options":[{"name":"(test_option)","value":"option_value"},{"name":"(test_option_2)","value":"option_value_3"}]}]},{"name":"ChannelChanger","rpcs":[{"name":"Next","in_type":"NextRequest","out_type":"Channel","in_streamed":true},{"name":"Previous","in_type":"PreviousRequest","out_type":"Channel","out_streamed":true}]}],"package":{"name":"dataset"},"options":[{"name":"java_multiple_files","value":"true"},{"name":"java_package","value":"test.java.package"},{"name":"java_outer_classname","value":"TestClass"}]}}]},"plugin_warnings":[{"filepath":"testdata/getProtoFiles/exclude/test.proto","message":"A sample warning!"},{"filepath":"testdata/getProtoFiles/exclude/test.proto","message":"Another sample warning.. ah!"}],"plugin_error_message":"some error"}

This is accurate, according to what the function should return, but I'm wondering if it's useful in the current state. Do you anticipate a user to parse this programmatically? I don't mind spitting it out for debug purposes, but if the intention was to include the JSON so that it can be decoded by an error handler, we might need to dig a bit further.

[milton0825]

I think the message is useful in case we enter the codepath in: https://github.com/nilslice/protolock/pull/113/files#diff-46f2fc221fcb719cdbdbb78a314f6c64R85

when the plugin does not work properly and exit

[nilslice]

Ok, thanks. So the JSON added is a nice way to see the data the plugin was fed at a snapshot when the plugin failed? I'm good with that if it is helpful.

[milton0825]

Yeah that is helpful. Also in the case when there is a stackoverflow in the plugin, the stacktrace will be available through the output as it gathers both stdout and stderr.

[nilslice]

As a follow-on, do you think it would be more helpful if there was an easier way to guarantee that the output could be more easily parsed? Splitting the output with a \n so that it becomes:

plugin-sample-error: some error (/go/bin/plugin-sample-error)
{"current":{"definitions":[{"protopath":"testdata:/:getProtoFiles:/:exclude:/:test.proto","def":{"messages":[{"name":"Test","fields":[{"id":1,"name":"name","t...}

would enable the user to decode the message if they were to build in some programmatic handling or better reporting in their CI pipeline.

it's probably not solving a known edge case, but I personally would like to be able to parse the output JSON from another tool if I pipe protolock into something in my CI... what do you think?

[nilslice]

While we're in this part of the code, I may also change the output to read:

plugin-sample-error (/go/bin/plugin-sample-error): some error 

Switching the order of the error message so it is last, after the plugin name and then it's location on the system seems nicer..

[milton0825]

Yeah I think we can make the error message easier to parse.

[nilslice]

Yeah I think we can make the error message easier to parse.

Let me know if you have a better suggestion than the newline, otherwise I'm happy to implement and create a new PR with your changes included.