> On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 145 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429940#file1429940line145> > > > > typo
Fixed thanks! > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 146 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429940#file1429940line146> > > > > The name appears to be too generic for its highly specialized use-case. > > Perhaps something along the lines of the "MesosFetcherUri" should be a > > better fit here? Since it mirrors the URI in the proto maybe MesosURI would do? I'll change it in the next submission if this is fine. > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 260 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429942#file1429942line260> > > > > Please revert, there should be a newline after the args spillover. Done. > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 266 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429942#file1429942line266> > > > > Favor java8 stream manipulations over Guava's where possible. > > > > Also, formatting is off, should be: > > ``` > > Iterable<Protos.CommandInfo.URI> uris = Iterables.transform( > > task.getTask().getUris(), > > u -> Protos.CommandInfo.URI.newBuilder() > > .setValue(u.getValue()) > > .setExecutable(false) > > .setExtract(u.isExtract()) > > .setCache(u.isCache()).build()); > > ``` Changed to Java 8 streams. > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 268 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429942#file1429942line268> > > > > This seems like a candidate for a command line flag documenting the > > direct security implications of changing the value here. > > > > Also, along the lines of Joshua's comments, suggest having another > > command line flag turning the feature off entirely (default) with a check > > in ConfigurationManager rejecting any tasks with fetcher URIs set. One flag to allow the executable field to be toggled and another to allow URIs to be fetched right? I've been playing it safe and not allowing the user a choice for now to set anything as executable as anything that needs to be downloaded to the sandbox and marked executable can be set to be fetched in the static server side config (custom executor config). I can add the ability to toggle being able to mark URIs downlaoded exectuable otherwise though. I've added the flag to turn this feature off entirely and have set it to be the default. > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java, > > lines 155-157 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429944#file1429944line155> > > > > Fits on a single line. Changed, thanks! > On June 27, 2016, 9:44 a.m., Maxim Khutornenko wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml, > > line 406 > > <https://reviews.apache.org/r/49218/diff/1/?file=1429947#file1429947line406> > > > > Please, add/modify AbstractTaskStoreTest to test this codepath. Added, will iterate if it's not good enough. - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49218/#review139588 ----------------------------------------------------------- On June 27, 2016, 4:56 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49218/ > ----------------------------------------------------------- > > (Updated June 27, 2016, 4:56 p.m.) > > > Review request for Aurora. > > > Repository: aurora > > > Description > ------- > > Adding a URIs field to TaskConfig inside the ThriftAPI so that users are able > to specify resources they wish to download into the sandbox per job. > > > Diffs > ----- > > RELEASE-NOTES.md af2061c7605c12a066778bd99ec1a3857bee6dec > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 3e6daf444453dd563dd7a2d494cc95e9a0aba0b6 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 6c7c75ac86458884bc767736caf47fb777756fc8 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 4089b79da8079243703eead884e80bcf736f8b29 > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > fe18c0f9876a73eeee6ea34d4eab92219013cd1e > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 3b01801d929dd61ee989495bf38af8f03e9f5ad4 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java > c76164292cf62d2181374c09f8bf6d8d3358e982 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java > 571201094c1e576e496495a01cb83f6c57decfa8 > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V007_CreateMesosFetcherURIsTable.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java > a90cb00e240df25dce6d55728859768e22d741a6 > > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml > 2c8af8b88e41b3b381cac831fd43b1057e4df0aa > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > 5069bedc08bb7111d0e0f101c8a2c81495b97bc9 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 2dff80b5213e98c778d71955517e5f9227d7d0c1 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 58785bfa37ff214f26e9f94d836e6df40e411c3b > > src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java > b1593f682f48ea66339bc2372de3e4f14e40be32 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > a883b0e33bfec1d14e6fe4ee8ed2200d93acaeec > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > ed69996593f5556d80a9229063ef5c7d658e2cfb > > Diff: https://reviews.apache.org/r/49218/diff/ > > > Testing > ------- > > ./gradlew build -Pq > ./build-support/jenkins/build.sh > bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Renan DelValle > >
