Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote Execution allows extra platform properties to be set #7650

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

commented May 2, 2019

Google's Remote Build Execution service now requires setting a platform property to specify in which docker
container you want to run actions.

@illicitonion illicitonion requested review from stuhood and Eric-Arellano May 2, 2019

@stuhood

stuhood approved these changes May 2, 2019

Copy link
Member

left a comment

Thanks!

@@ -2151,7 +2244,7 @@ mod tests {
let op_name = "gimme-foo".to_string();
mock::execution_server::TestServer::new(mock::execution_server::MockExecution::new(
op_name.clone(),
super::make_execute_request(&execute_request, &None, &None)
super::make_execute_request(&execute_request, &None, &None, BTreeMap::new())

This comment has been minimized.

Copy link
@stuhood

stuhood May 2, 2019

Member

Since specifying platform properties is rare, is it worth "overloading" with a longer name to specify them?

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 2, 2019

Author Contributor

We already have two optional fields here I'd want to do that for, so I don't really want to introduce one "overload" :( Maybe one day we'll get default arguments...

This comment has been minimized.

Copy link
@stuhood

stuhood May 2, 2019

Member

This... doesn't feel like a great argument for continuing to expand the mostly-not-used arguments in order to have just one method. But not a blocker.

illicitonion added some commits May 2, 2019

Remote Execution allows extra platform properties to be set
RBE now requires setting a platform property to specify in which docker
container you want to run actions.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/remoteexecution/platformproperties branch from 00f21e3 to a4b2d2e May 2, 2019

@illicitonion illicitonion merged commit c9be6bc into pantsbuild:master May 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/remoteexecution/platformproperties branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.