Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-208

Location should have a way to set permissions when creating directories

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.10.0
    • Component/s: None
    • Labels:
      None

      Description

      That is, we need to introduce a method

      boolean mkdirs(String permissions);
      

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user anew opened a pull request:

          https://github.com/apache/twill/pull/26

          (TWILL-208) add Location.mkdirs(String permissions)

          • adds mkdirs() that takes permissions
          • makes sure getOutputStream(permisssions) also uses that when creating the directories
          • add test case

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/anew/twill twill-208

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/26.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #26


          commit 6d32277cc7cf03afd11e9bbef4d382fccb95d4ea
          Author: anew <andreas@cask.co>
          Date: 2017-01-24T23:39:43Z

          (TWILL-208) add Location.mkdirs(String permissions)


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user anew opened a pull request: https://github.com/apache/twill/pull/26 ( TWILL-208 ) add Location.mkdirs(String permissions) adds mkdirs() that takes permissions makes sure getOutputStream(permisssions) also uses that when creating the directories add test case You can merge this pull request into a Git repository by running: $ git pull https://github.com/anew/twill twill-208 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/26.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #26 commit 6d32277cc7cf03afd11e9bbef4d382fccb95d4ea Author: anew <andreas@cask.co> Date: 2017-01-24T23:39:43Z ( TWILL-208 ) add Location.mkdirs(String permissions)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97681685

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java —
          @@ -207,6 +207,21 @@
          boolean mkdirs() throws IOException;

          /**
          + * Creates the directory named by this abstract pathname, including any necessary
          + * but nonexistent parent directories.
          + *
          + * @param permission A permission string. It has to be either a three digit or a nine character string.
          + * For the three digit string, it is similar to the UNIX permission numeric representation.
          + * The first digit is the permission for owner, second digit is the permission for group and
          + * the third digit is the permission for all.
          + * For the nine character string, it uses the format as specified by the
          + *

          {@link PosixFilePermissions#fromString(String)}

          method.
          + *
          + * @return true if and only if the renaming succeeded; false otherwise
          — End diff –

          update javadoc

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97681685 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java — @@ -207,6 +207,21 @@ boolean mkdirs() throws IOException; /** + * Creates the directory named by this abstract pathname, including any necessary + * but nonexistent parent directories. + * + * @param permission A permission string. It has to be either a three digit or a nine character string. + * For the three digit string, it is similar to the UNIX permission numeric representation. + * The first digit is the permission for owner, second digit is the permission for group and + * the third digit is the permission for all. + * For the nine character string, it uses the format as specified by the + * {@link PosixFilePermissions#fromString(String)} method. + * + * @return true if and only if the renaming succeeded; false otherwise — End diff – update javadoc
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97683616

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException

          { return file.mkdirs(); }

          + @Override
          + public boolean mkdirs(String permission) throws IOException {
          — End diff –

          Should we use [Files.createDirectories](http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createDirectories(java.nio.file.Path,%20java.nio.file.attribute.FileAttribute...)) since it also takes permissions?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97683616 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException { return file.mkdirs(); } + @Override + public boolean mkdirs(String permission) throws IOException { — End diff – Should we use [Files.createDirectories] ( http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createDirectories(java.nio.file.Path,%20java.nio.file.attribute.FileAttribute ...)) since it also takes permissions?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97682185

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java —
          @@ -207,6 +207,21 @@
          boolean mkdirs() throws IOException;

          /**
          + * Creates the directory named by this abstract pathname, including any necessary
          + * but nonexistent parent directories.
          + *
          + * @param permission A permission string. It has to be either a three digit or a nine character string.
          + * For the three digit string, it is similar to the UNIX permission numeric representation.
          + * The first digit is the permission for owner, second digit is the permission for group and
          + * the third digit is the permission for all.
          + * For the nine character string, it uses the format as specified by the
          + *

          {@link PosixFilePermissions#fromString(String)}

          method.
          + *
          + * @return true if and only if the renaming succeeded; false otherwise
          — End diff –

          `@return true if and only if the renaming succeeded; false otherwise` => needs to change from renaming to mkdir

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97682185 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java — @@ -207,6 +207,21 @@ boolean mkdirs() throws IOException; /** + * Creates the directory named by this abstract pathname, including any necessary + * but nonexistent parent directories. + * + * @param permission A permission string. It has to be either a three digit or a nine character string. + * For the three digit string, it is similar to the UNIX permission numeric representation. + * The first digit is the permission for owner, second digit is the permission for group and + * the third digit is the permission for all. + * For the nine character string, it uses the format as specified by the + * {@link PosixFilePermissions#fromString(String)} method. + * + * @return true if and only if the renaming succeeded; false otherwise — End diff – `@return true if and only if the renaming succeeded; false otherwise` => needs to change from renaming to mkdir
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97682463

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -98,8 +98,8 @@ public OutputStream getOutputStream() throws IOException {

          @Override
          public OutputStream getOutputStream(String permission) throws IOException {

          • ensureDirectory(file.getParentFile());
            Set<PosixFilePermission> permissions = parsePermissions(permission);
            + ensureDirectory(file.getParentFile(), permissions);
              • End diff –

          Do we want to use the file permissions for the directory too?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97682463 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -98,8 +98,8 @@ public OutputStream getOutputStream() throws IOException { @Override public OutputStream getOutputStream(String permission) throws IOException { ensureDirectory(file.getParentFile()); Set<PosixFilePermission> permissions = parsePermissions(permission); + ensureDirectory(file.getParentFile(), permissions); End diff – Do we want to use the file permissions for the directory too?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97689666

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java —
          @@ -207,6 +207,21 @@
          boolean mkdirs() throws IOException;

          /**
          + * Creates the directory named by this abstract pathname, including any necessary
          + * but nonexistent parent directories.
          + *
          + * @param permission A permission string. It has to be either a three digit or a nine character string.
          + * For the three digit string, it is similar to the UNIX permission numeric representation.
          + * The first digit is the permission for owner, second digit is the permission for group and
          + * the third digit is the permission for all.
          + * For the nine character string, it uses the format as specified by the
          + *

          {@link PosixFilePermissions#fromString(String)}

          method.
          + *
          + * @return true if and only if the renaming succeeded; false otherwise
          — End diff –

          good catch. I actually did not want that line

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97689666 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java — @@ -207,6 +207,21 @@ boolean mkdirs() throws IOException; /** + * Creates the directory named by this abstract pathname, including any necessary + * but nonexistent parent directories. + * + * @param permission A permission string. It has to be either a three digit or a nine character string. + * For the three digit string, it is similar to the UNIX permission numeric representation. + * The first digit is the permission for owner, second digit is the permission for group and + * the third digit is the permission for all. + * For the nine character string, it uses the format as specified by the + * {@link PosixFilePermissions#fromString(String)} method. + * + * @return true if and only if the renaming succeeded; false otherwise — End diff – good catch. I actually did not want that line
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97689935

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -98,8 +98,8 @@ public OutputStream getOutputStream() throws IOException {

          @Override
          public OutputStream getOutputStream(String permission) throws IOException {

          • ensureDirectory(file.getParentFile());
            Set<PosixFilePermission> permissions = parsePermissions(permission);
            + ensureDirectory(file.getParentFile(), permissions);
              • End diff –

          yes, the intention of permissions is that others get access through, for example, group permission. So if I create the file with 700 but the directories are created with the default umask of 750, then that is not great. Even worse, if I create the file with 755, and the directories are created with 750, then world cannot access the file despite the intention.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97689935 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -98,8 +98,8 @@ public OutputStream getOutputStream() throws IOException { @Override public OutputStream getOutputStream(String permission) throws IOException { ensureDirectory(file.getParentFile()); Set<PosixFilePermission> permissions = parsePermissions(permission); + ensureDirectory(file.getParentFile(), permissions); End diff – yes, the intention of permissions is that others get access through, for example, group permission. So if I create the file with 700 but the directories are created with the default umask of 750, then that is not great. Even worse, if I create the file with 755, and the directories are created with 750, then world cannot access the file despite the intention.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97689959

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException

          { return file.mkdirs(); }

          + @Override
          + public boolean mkdirs(String permission) throws IOException {
          — End diff –

          oh, did not know about that. let me try.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97689959 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException { return file.mkdirs(); } + @Override + public boolean mkdirs(String permission) throws IOException { — End diff – oh, did not know about that. let me try.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97690501

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException

          { return file.mkdirs(); }

          + @Override
          + public boolean mkdirs(String permission) throws IOException {
          — End diff –

          hmm that does not seem to apply the permissions correctly, it applies the umask to the permissions. What we want is the exact permissions as passed in.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97690501 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException { return file.mkdirs(); } + @Override + public boolean mkdirs(String permission) throws IOException { — End diff – hmm that does not seem to apply the permissions correctly, it applies the umask to the permissions. What we want is the exact permissions as passed in.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97855245

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -47,4 +52,14 @@ public static void finish() {
          protected LocationFactory createLocationFactory(String pathBase) throws Exception

          { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); }

          +
          + // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          + @Ignore
          + @Test(expected = Exception.class)
          + public void testPermissions() throws IOException {
          + // create a directory that does not permit anything
          + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          — End diff –

          How do we know for sure that the exception was not thrown by this operation?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97855245 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -47,4 +52,14 @@ public static void finish() { protected LocationFactory createLocationFactory(String pathBase) throws Exception { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); } + + // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions + @Ignore + @Test(expected = Exception.class) + public void testPermissions() throws IOException { + // create a directory that does not permit anything + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); — End diff – How do we know for sure that the exception was not thrown by this operation?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97874790

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java —
          @@ -207,6 +207,21 @@
          boolean mkdirs() throws IOException;

          /**
          + * Creates the directory named by this abstract pathname, including any necessary
          + * but nonexistent parent directories.
          + *
          + * @param permission A permission string. It has to be either a three digit or a nine character string.
          + * For the three digit string, it is similar to the UNIX permission numeric representation.
          + * The first digit is the permission for owner, second digit is the permission for group and
          + * the third digit is the permission for all.
          + * For the nine character string, it uses the format as specified by the
          + *

          {@link PosixFilePermissions#fromString(String)}

          method.
          + *
          + * @return true if and only if the renaming succeeded; false otherwise
          — End diff –

          done

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97874790 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/Location.java — @@ -207,6 +207,21 @@ boolean mkdirs() throws IOException; /** + * Creates the directory named by this abstract pathname, including any necessary + * but nonexistent parent directories. + * + * @param permission A permission string. It has to be either a three digit or a nine character string. + * For the three digit string, it is similar to the UNIX permission numeric representation. + * The first digit is the permission for owner, second digit is the permission for group and + * the third digit is the permission for all. + * For the nine character string, it uses the format as specified by the + * {@link PosixFilePermissions#fromString(String)} method. + * + * @return true if and only if the renaming succeeded; false otherwise — End diff – done
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97875317

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -47,4 +52,14 @@ public static void finish() {
          protected LocationFactory createLocationFactory(String pathBase) throws Exception

          { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); }

          +
          + // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          + @Ignore
          + @Test(expected = Exception.class)
          + public void testPermissions() throws IOException {
          + // create a directory that does not permit anything
          + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          — End diff –

          Good point. Anyway, this test does not throw an exception and is ignored because of that. I guess this is a typical problem with using expected=, we can never know whether it is thrown by the line in the code where we expect. So should we rewrite all our test cases not to use that? And explicitly fail if the exception is not thrown?

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97875317 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -47,4 +52,14 @@ public static void finish() { protected LocationFactory createLocationFactory(String pathBase) throws Exception { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); } + + // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions + @Ignore + @Test(expected = Exception.class) + public void testPermissions() throws IOException { + // create a directory that does not permit anything + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); — End diff – Good point. Anyway, this test does not throw an exception and is ignored because of that. I guess this is a typical problem with using expected=, we can never know whether it is thrown by the line in the code where we expect. So should we rewrite all our test cases not to use that? And explicitly fail if the exception is not thrown?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97876853

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -47,4 +52,14 @@ public static void finish() {
          protected LocationFactory createLocationFactory(String pathBase) throws Exception

          { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); }

          +
          + // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          + @Ignore
          + @Test(expected = Exception.class)
          + public void testPermissions() throws IOException {
          + // create a directory that does not permit anything
          + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          — End diff –

          Oh ok, missed the Ignore annotation. It might be good to try and catch and have an Assert.fail at the end of the try block for the new tests that we are adding.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97876853 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -47,4 +52,14 @@ public static void finish() { protected LocationFactory createLocationFactory(String pathBase) throws Exception { return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase); } + + // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions + @Ignore + @Test(expected = Exception.class) + public void testPermissions() throws IOException { + // create a directory that does not permit anything + dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); — End diff – Oh ok, missed the Ignore annotation. It might be good to try and catch and have an Assert.fail at the end of the try block for the new tests that we are adding.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97880069

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio

          // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          @Ignore

          • @Test(expected = Exception.class)
            public void testPermissions() throws IOException {
            // create a directory that does not permit anything
            dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          • // creating a subdir should fail because even the owner has no write permission
          • dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------"));
            + boolean succeeded = false;
            + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + }

            catch (Exception e)

            { + // expected; TODO: figure out which exception should the expected here, it's not documented + }

            + if (succeeded) {

              • End diff –

          nit: you can move the Assert.fail to line 65 and remove the boolean variable.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97880069 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions @Ignore @Test(expected = Exception.class) public void testPermissions() throws IOException { // create a directory that does not permit anything dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); // creating a subdir should fail because even the owner has no write permission dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + boolean succeeded = false; + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + } catch (Exception e) { + // expected; TODO: figure out which exception should the expected here, it's not documented + } + if (succeeded) { End diff – nit: you can move the Assert.fail to line 65 and remove the boolean variable.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97880251

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio

          // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          @Ignore

          • @Test(expected = Exception.class)
            public void testPermissions() throws IOException {
            // create a directory that does not permit anything
            dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          • // creating a subdir should fail because even the owner has no write permission
          • dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------"));
            + boolean succeeded = false;
            + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + }

            catch (Exception e)

            { + // expected; TODO: figure out which exception should the expected here, it's not documented + }

            + if (succeeded) {

              • End diff –

          no I can't because line 66 would catch it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97880251 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions @Ignore @Test(expected = Exception.class) public void testPermissions() throws IOException { // create a directory that does not permit anything dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); // creating a subdir should fail because even the owner has no write permission dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + boolean succeeded = false; + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + } catch (Exception e) { + // expected; TODO: figure out which exception should the expected here, it's not documented + } + if (succeeded) { End diff – no I can't because line 66 would catch it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97885713

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio

          // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          @Ignore

          • @Test(expected = Exception.class)
            public void testPermissions() throws IOException {
            // create a directory that does not permit anything
            dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          • // creating a subdir should fail because even the owner has no write permission
          • dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------"));
            + boolean succeeded = false;
            + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + }

            catch (Exception e)

            { + // expected; TODO: figure out which exception should the expected here, it's not documented + }

            + if (succeeded) {

              • End diff –

          No it won't I think. Since Assert.fail throws AssertionError which doesn't extend Exception?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97885713 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions @Ignore @Test(expected = Exception.class) public void testPermissions() throws IOException { // create a directory that does not permit anything dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); // creating a subdir should fail because even the owner has no write permission dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + boolean succeeded = false; + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + } catch (Exception e) { + // expected; TODO: figure out which exception should the expected here, it's not documented + } + if (succeeded) { End diff – No it won't I think. Since Assert.fail throws AssertionError which doesn't extend Exception?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r97929702

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java —
          @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio

          // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce permissions
          @Ignore

          • @Test(expected = Exception.class)
            public void testPermissions() throws IOException {
            // create a directory that does not permit anything
            dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------"));
          • // creating a subdir should fail because even the owner has no write permission
          • dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------"));
            + boolean succeeded = false;
            + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + }

            catch (Exception e)

            { + // expected; TODO: figure out which exception should the expected here, it's not documented + }

            + if (succeeded) {

              • End diff –

          oh right

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r97929702 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java — @@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String pathBase) throws Exceptio // TODO ( TWILL-209 ): figure out how to make MiniDFSCluster enforce permissions @Ignore @Test(expected = Exception.class) public void testPermissions() throws IOException { // create a directory that does not permit anything dfsCluster.getFileSystem().mkdir(new Path("/a"), FsPermission.valueOf("----------")); // creating a subdir should fail because even the owner has no write permission dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + boolean succeeded = false; + try { + // creating a subdir should fail because even the owner has no write permission + dfsCluster.getFileSystem().mkdir(new Path("/a/b"), FsPermission.valueOf("----------")); + succeeded = true; + } catch (Exception e) { + // expected; TODO: figure out which exception should the expected here, it's not documented + } + if (succeeded) { End diff – oh right
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

          https://github.com/apache/twill/pull/26

          LGTM 👍

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/26 LGTM 👍
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r98070266

          — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java —
          @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException

          { return file.mkdirs(); }

          + @Override
          + public boolean mkdirs(String permission) throws IOException {
          — End diff –

          I see, in that case I think it would be good to add this to the javadoc of the method.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r98070266 — Diff: twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java — @@ -251,6 +251,34 @@ public boolean mkdirs() throws IOException { return file.mkdirs(); } + @Override + public boolean mkdirs(String permission) throws IOException { — End diff – I see, in that case I think it would be good to add this to the javadoc of the method.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on the issue:

          https://github.com/apache/twill/pull/26

          Just one comment on javadoc, please fix and merge 👍

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on the issue: https://github.com/apache/twill/pull/26 Just one comment on javadoc, please fix and merge 👍
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on the issue:

          https://github.com/apache/twill/pull/26

          LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on the issue: https://github.com/apache/twill/pull/26 LGTM
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on the issue:

          https://github.com/apache/twill/pull/26

          Squashing commits now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on the issue: https://github.com/apache/twill/pull/26 Squashing commits now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/26#discussion_r98118427

          — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/FileContextLocationTest.java —
          @@ -17,26 +17,36 @@
          */
          package org.apache.twill.filesystem;

          +import com.google.common.base.Throwables;
          import org.apache.hadoop.conf.Configuration;
          +import org.apache.hadoop.fs.Path;
          +import org.apache.hadoop.fs.permission.FsPermission;
          import org.apache.hadoop.hdfs.MiniDFSCluster;
          +import org.apache.hadoop.security.UserGroupInformation;
          import org.junit.AfterClass;
          import org.junit.BeforeClass;

          import java.io.IOException;
          +import java.security.PrivilegedAction;

          /**
          *
          */
          public class FileContextLocationTest extends LocationTestBase {

          public static MiniDFSCluster dfsCluster;
          + private static UserGroupInformation testUGI;

          @BeforeClass
          public static void init() throws IOException {
          Configuration conf = new Configuration();

          • conf.setBoolean("hdfs.permissions", true);
            conf.set(MiniDFSCluster.HDFS_MINIDFS_BASEDIR, tmpFolder.newFolder().getAbsolutePath());
            dfsCluster = new MiniDFSCluster.Builder(conf).numDataNodes(1).build();
            + // make rooot world-writable so that we can create all location factories as unprivileged user
              • End diff –

          root

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/26#discussion_r98118427 — Diff: twill-yarn/src/test/java/org/apache/twill/filesystem/FileContextLocationTest.java — @@ -17,26 +17,36 @@ */ package org.apache.twill.filesystem; +import com.google.common.base.Throwables; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hdfs.MiniDFSCluster; +import org.apache.hadoop.security.UserGroupInformation; import org.junit.AfterClass; import org.junit.BeforeClass; import java.io.IOException; +import java.security.PrivilegedAction; /** * */ public class FileContextLocationTest extends LocationTestBase { public static MiniDFSCluster dfsCluster; + private static UserGroupInformation testUGI; @BeforeClass public static void init() throws IOException { Configuration conf = new Configuration(); conf.setBoolean("hdfs.permissions", true); conf.set(MiniDFSCluster.HDFS_MINIDFS_BASEDIR, tmpFolder.newFolder().getAbsolutePath()); dfsCluster = new MiniDFSCluster.Builder(conf).numDataNodes(1).build(); + // make rooot world-writable so that we can create all location factories as unprivileged user End diff – root
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on the issue:

          https://github.com/apache/twill/pull/26

          LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on the issue: https://github.com/apache/twill/pull/26 LGTM
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on the issue:

          https://github.com/apache/twill/pull/26

          squashing one more time

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on the issue: https://github.com/apache/twill/pull/26 squashing one more time
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/26

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/26

            People

            • Assignee:
              anew Andreas Neumann
              Reporter:
              anew Andreas Neumann
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development