You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/02/25 06:41:39 UTC

[GitHub] [zeppelin] BagusThanatos opened a new pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

BagusThanatos opened a new pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298


   ### What is this PR for?
   This is to implement move function that is used for renaming folders and moving it to trash.
   Currently it would result in a no op, making users unable to rename folders if using GCS as the notebook repository.
   
   
   ### What type of PR is it?
   Feature
   
   ### What is the Jira issue?
   [* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
   * Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533]
   ](https://issues.apache.org/jira/browse/ZEPPELIN-5660)
   
   ### How should this be tested?
   - CI
   - A custom jar using this patch is currently being used in our Zeppelin instance
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] BagusThanatos commented on a change in pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
BagusThanatos commented on a change in pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#discussion_r814569404



##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -187,6 +187,28 @@ public void testRemove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testRemoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.remove("id", "/name", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       If I removed the try catch block, then the test would fail.
   
   The purpose of this test is to make sure that the function would throw an exception if it cannot remove the folder, resulting in the `fail()` part of the test to never be reached.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] BagusThanatos commented on a change in pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
BagusThanatos commented on a change in pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#discussion_r814590258



##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -187,6 +187,28 @@ public void testRemove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testRemoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.remove("id", "/name", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       I see
   
   That can do
   
   I was following the previous tests written in the class
   
   Changing it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#issuecomment-1055502643


   LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#discussion_r814559922



##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -187,6 +187,28 @@ public void testRemove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testRemoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.remove("id", "/name", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       try catch block is not necessary.

##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -202,6 +224,29 @@ public void testMove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testMoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.move("/name", "/name_new", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       try catch block is not necessary.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu merged pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
zjffdu merged pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#discussion_r814585182



##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -187,6 +187,28 @@ public void testRemove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testRemoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.remove("id", "/name", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       Perhaps the following article will help you. This shows you how your test expects an exception.
   https://www.baeldung.com/junit-assert-exception
   
   I would prefer 
   `@Test(expected = IOException.class)`
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] BagusThanatos commented on a change in pull request #4298: [ZEPPELIN-5660] Implement move gcsnotebookrepo folder

Posted by GitBox <gi...@apache.org>.
BagusThanatos commented on a change in pull request #4298:
URL: https://github.com/apache/zeppelin/pull/4298#discussion_r814592195



##########
File path: zeppelin-plugins/notebookrepo/gcs/src/test/java/org/apache/zeppelin/notebook/repo/GCSNotebookRepoTest.java
##########
@@ -187,6 +187,28 @@ public void testRemove() throws Exception {
     assertThat(storage.get(makeBlobId(runningNote.getId(), runningNote.getPath()))).isNull();
   }
 
+  @Test
+  public void testRemoveFolder_nonexistent() throws Exception {
+    try {
+      notebookRepo.remove("id", "/name", AUTH_INFO);
+      fail();
+    } catch (IOException e) {}

Review comment:
       @Reamer 
   Updated
   Please check again
   Thanks!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org