On 7/19/19 10:13 AM, Severin Gehwolf wrote:
Hi Mandy,
On Fri, 2019-07-19 at 09:58 -0700, Mandy Chung wrote:
Hi Severin,
On 7/19/19 9:55 AM, Severin Gehwolf wrote:
There might be other tests with policy files where this is not the case.
My issue is with finding those tests :-/ If we know the set of *all*
tests affected by the breakage we could do approach 2. Approach 1 (or
3) seems safer.
Severin - how about a combination of the two approaches, meaning add
Docker.DOCKER_COMMAND as per the first version but use
privilegedGetProperty to read the value. That way only container tests
using a SM and their own policy files will need to grant the permission
to read this property.
Sure, fine with me. Here you go:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8228434/02/webrev/
It seems better to define Docker as a new top-level class as Platform
doesn't seem the right place to define DOCKER_COMMAND.
Note that Platform.java is already on the boot library path for jtreg
and moving it to a separate class would need another entry here (in
TEST.ROOT):
requires.extraPropDefns.bootlibs = ../lib/sun ../lib/jdk/test/lib/Platform.java
The reason I've moved it to Platform for JDK-8227642 was that
VMProps.java previously hard-coded the docker command (it needs to use
the property value instead).
It accepts multiple files, doesn't it?
This seems work for little gain. Note that the intention is to change
"Docker" and DOCKER_COMMAND to something more neutral, like Container
or ContainerEngine. In light of that, Platform.ContainerEngine.COMMAND
makes sense to me.
This is a test library specific to containers. "Container" sounds fine.
Why not doing it now?
Could I perhaps convince you to accept this version?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8228434/02/webrev/
I still think separating container-specific APIs in its own class will
prevent running similar kind of issue in the future.
Mandy
