Move output_node_properties from Action to Command

[juergbi]

This is consistent with the other output_* fields.

[juergbi]

As discussed last Tuesday, I will prepare another PR to replace MTime and UnixMode defined in the lexicon with regular Protobuf fields. However, looking into that I've noticed that output_node_properties is part of the Action message while all the other output-related fields were moved to the Command message a while ago. I think it would make more sense if output_node_properties and the new field (equivalent of output_node_properties for the new Protobuf fields) were in the Command message as well.

I've marked Action.output_node_properties as deprecated and servers can still support it if they care about clients that use it. Alternatively, we could also replace Action.output_node_properties with a reserved field as we don't expect significant use in the wild yet.

[bergsieker]

I think it's better to go the RESERVED route if we can--this field was recently added, and it's not clear that anyone is actually using it yet. Can you follow up with the various client and server implementations to make sure it's not being used and then replace the original field with a RESERVED one?

[bergsieker]

Note: Bazel, the remote-apis-sdk, and goma are not using this field.

[bergsieker]

Also, this should regenerate the related go files. Ola thinks that should have been done by the commit hook, but for some reason in this case it was not?

[sstriker]

Only if you actually configure your clone to use the hook.

…

On Fri, Mar 6, 2020 at 7:40 PM Steven Bergsieker ***@***.***> wrote: Also, this *should* regenerate the related go files. Ola thinks that should have been done by the commit hook, but for some reason in this case it was not? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#121?email_source=notifications&email_token=AAQVXZ7Q44RSMJSXWTEBQVTRGE7QTA5CNFSM4KWVFJJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOCMMSQ#issuecomment-595904074>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQVXZZZFFZIXSKBNUGW4ILRGE7QTANCNFSM4KWVFJJA> .

[juergbi]

I think it's better to go the RESERVED route if we can--this field was recently added, and it's not clear that anyone is actually using it yet. Can you follow up with the various client and server implementations to make sure it's not being used and then replace the original field with a RESERVED one?

Sounds fine with me. BuildStream and BuildBox master support it but this change shouldn't cause any disruption to users. Buck, Buildbarn, Buildfarm, BuildGrid, Pants and RECC don't use it, based on grepping the repositories. I don't know about RBE but I'd be surprised if it uses it. Any other component we should check?

[juergbi]

I've mentioned this PR in the monthly meeting on Tuesday and there has been no objection. Is this now ok to be merged?