reapi: favor platform set in Action over Command #7550
Conversation
In REAPI v2.2, the usage of platform in Command was deprecated over in Action. Although most of the older REAPI clients, such as Bazel, are setting it in both, newer clients such as Buck2 would only set it in Action message. This change replace most of existing `command.GetPlatform()` calls with `action.GetPlatform()` while using the command value as fallback. Closes buildbuddy-io/buildbuddy-internal#3863
There was a problem hiding this comment.
as a preliminary step, wdyt about adding some logging in execution_server to see whether we get any actions where the action platform is empty but the command platform isn't? If in practice, the action platform is always set, then I think we could skip adding this fallback logic everywhere to simplify things a bit. (I'm fine with the fallback as well, just thinking if we want to remove the fallback eventually anyway, we may as well just do it right away, if possible?)
| p := task.GetAction().GetPlatform() | ||
| if p == nil || len(p.GetProperties()) == 0 { | ||
| p = task.GetCommand().GetPlatform() | ||
| } |
There was a problem hiding this comment.
nit: can you consolidate this fallback logic into a helper func in platform.go like platform.GetProto(action, command)
| // FindValueInTask scans the platform properties of the given task for the given | ||
| // property name (ignoring case) and returns the value of that property if it | ||
| // exists, otherwise "". | ||
| func FindValueInTask(task *repb.ExecutionTask, name string) string { | ||
| p := task.GetAction().GetPlatform() | ||
| if p == nil || len(p.GetProperties()) == 0 { | ||
| p = task.GetCommand().GetPlatform() | ||
| } | ||
| value, _ := findValue(p, name) | ||
| return value | ||
| } |
There was a problem hiding this comment.
nit: I think if we had a helper func like platform.GetProto(action, command) that returns the effective platform, then we could instead write p := platform.GetProto(task.GetAction(), task.GetCommand()) and platform.FindValue(p, "some-prop") instead of using this helper func. It's slightly more verbose but helps minimize the API surface of this package - and we only need this logic in a couple of places.
In REAPI v2.2, the usage of platform in Command was deprecated over in Action. Although most of the older REAPI clients, such as Bazel, are setting it in both, newer clients such as Buck2 would only set it in Action message. This change replace most of existing command.GetPlatform() calls with action.GetPlatform() while using the command value as fallback. This was heavily inspired by #7550, but with a few more changes. I tried a simpler approach where we overwrite command.platform with action.platform, but because we read the original protos by digest in ExecutionServer.markTaskComplete, this doesn't work. Closes buildbuddy-io/buildbuddy-internal#3863
In REAPI v2.2, the usage of platform in Command was deprecated over in Action. Although most of the older REAPI clients, such as Bazel, are setting it in both, newer clients such as Buck2 would only set it in Action message. This change replace most of existing command.GetPlatform() calls with action.GetPlatform() while using the command value as fallback. This was heavily inspired by #7550, but with a few more changes. I tried a simpler approach where we overwrite command.platform with action.platform, but because we read the original protos by digest in ExecutionServer.markTaskComplete, this doesn't work. Closes buildbuddy-io/buildbuddy-internal#3863
In REAPI v2.2, the usage of platform in Command was deprecated over in
Action. Although most of the older REAPI clients, such as Bazel, are
setting it in both, newer clients such as Buck2 would only set it in
Action message.
This change replace most of existing
command.GetPlatform()calls withaction.GetPlatform()while using the command value as fallback.Closes https://github.com/buildbuddy-io/buildbuddy-internal/issues/3863