Fixes after review round

5 files changed, 29 insertions, 34 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystem.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystem.java
index 65657aae472..c5c2df9e38e 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystem.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystem.java
@@ -52,16 +52,8 @@ public class FileSystem {
                 Files.getFileAttributeView(path, PosixFileAttributeView.class).readAttributes());
     }
 
-    public Set<PosixFilePermission> getPermissions(Path path) {
-        return getAttributes(path).permissions();
-    }
-
-    static void validatePermissions(String permissions) {
-        Set<PosixFilePermission> set = PosixFilePermissions.fromString(permissions);
-        String serialized = PosixFilePermissions.toString(set);
-        if (!permissions.equals(serialized)) {
-            throw new IllegalArgumentException("Bad persmissions string: " + permissions);
-        }
+    public String getPermissions(Path path) {
+        return PosixFilePermissions.toString(getAttributes(path).permissions());
     }
 
     /**
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemPath.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemPath.java
index 552b827a255..bfec341e05c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemPath.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemPath.java
@@ -4,8 +4,6 @@ package com.yahoo.vespa.hosted.node.admin.io;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.attribute.FileAttribute;
-import java.nio.file.attribute.PosixFilePermission;
-import java.util.Set;
 
 /**
  * Convenience class for calling FileSystem methods on a fixed Path.
@@ -41,7 +39,7 @@ public class FileSystemPath {
         return this;
     }
 
-    public Set<PosixFilePermission> getPermissions() {
+    public String getPermissions() {
         return fileSystem.getPermissions(path);
     }
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/MakeDirectoryTask.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/MakeDirectoryTask.java
index 4b1ca0bd2bc..522abb81248 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/MakeDirectoryTask.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/MakeDirectoryTask.java
@@ -6,11 +6,11 @@ import com.yahoo.vespa.hosted.node.admin.io.FileSystem;
 import java.nio.file.Path;
 
 public class MakeDirectoryTask implements Task {
-    private final Path directory;
+    private final Path path;
     private boolean withParents = false;
 
-    public MakeDirectoryTask(Path directory) {
-        this.directory = directory;
+    public MakeDirectoryTask(Path path) {
+        this.path = path;
     }
 
     public MakeDirectoryTask withParents() {
@@ -18,6 +18,14 @@ public class MakeDirectoryTask implements Task {
         return this;
     }
 
+    Path getPath() {
+        return path;
+    }
+
+    boolean getWithParents() {
+        return withParents;
+    }
+
     private boolean makeDirectory(FileSystem fileSystem,
                                   Path directory,
                                   boolean withParents) {
@@ -36,6 +44,6 @@ public class MakeDirectoryTask implements Task {
 
     @Override
     public boolean execute(TaskContext context) {
-        return makeDirectory(context.getFileSystem(), directory, withParents);
+        return makeDirectory(context.getFileSystem(), path, withParents);
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemTest.java
index 91a5ccdfb27..6961efc159c 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/io/FileSystemTest.java
@@ -7,8 +7,6 @@ import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
 import java.nio.file.Path;
-import java.nio.file.attribute.PosixFilePermission;
-import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -59,17 +57,9 @@ public class FileSystemTest {
 
     @Test
     public void permissions() throws Exception {
-        fileSystem.setPermissions(path, "rwxr-x---");
-        Set<PosixFilePermission> permissions = fileSystem.getPermissions(path);
-        assertTrue(permissions.contains(PosixFilePermission.OWNER_READ));
-        assertTrue(permissions.contains(PosixFilePermission.OWNER_WRITE));
-        assertTrue(permissions.contains(PosixFilePermission.OWNER_EXECUTE));
-        assertTrue(permissions.contains(PosixFilePermission.GROUP_READ));
-        assertFalse(permissions.contains(PosixFilePermission.GROUP_WRITE));
-        assertTrue(permissions.contains(PosixFilePermission.GROUP_EXECUTE));
-        assertFalse(permissions.contains(PosixFilePermission.OTHERS_READ));
-        assertFalse(permissions.contains(PosixFilePermission.OTHERS_WRITE));
-        assertFalse(permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
+        String expectedPermissions = "rwxr-x---";
+        fileSystem.setPermissions(path, expectedPermissions);
+        assertEquals(expectedPermissions, fileSystem.getPermissions(path));
     }
 
     @Test(expected = IllegalArgumentException.class)
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/WriteFileTaskTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/WriteFileTaskTest.java
index 2294e9fed9d..9c998cc6fdb 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/WriteFileTaskTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/WriteFileTaskTest.java
@@ -8,6 +8,7 @@ import org.mockito.ArgumentCaptor;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
@@ -21,7 +22,8 @@ public class WriteFileTaskTest extends TaskTestBase {
 
     @Test
     public void testWrite() {
-        Path path = Paths.get("foo");
+        Path parentDirectory = Paths.get("/foo");
+        Path path = parentDirectory.resolve("bar");
 
         @SuppressWarnings("unchecked")
         Producer<String> contentProducer = (Producer<String>) mock(Producer.class);
@@ -41,16 +43,21 @@ public class WriteFileTaskTest extends TaskTestBase {
 
         assertTrue(task.execute(contextMock));
 
+        verify(contentProducer, times(1)).call();
         verify(fileSystemMock).writeUtf8File(path, content);
         verify(fileSystemMock).setPermissions(path, permissions);
         verify(fileSystemMock).setOwner(path, owner);
         verify(fileSystemMock).setGroup(path, group);
 
         // Writing a file with the expected content
-        ArgumentCaptor<WriteFileTask> writeFileTaskArgumentCaptor =
-                ArgumentCaptor.forClass(WriteFileTask.class);
+        ArgumentCaptor<MakeDirectoryTask> makeDirectoryTaskCaptor =
+                ArgumentCaptor.forClass(MakeDirectoryTask.class);
         verify(contextMock, times(1))
-                .executeSubtask(writeFileTaskArgumentCaptor.capture());
+                .executeSubtask(makeDirectoryTaskCaptor.capture());
+
+        MakeDirectoryTask makeDirectoryTask = makeDirectoryTaskCaptor.getValue();
+        assertEquals(parentDirectory, makeDirectoryTask.getPath());
+        assertTrue(makeDirectoryTask.getWithParents());
     }
 
     @Test