[zeppelin] branch master updated: [ZEPPELIN-5472] Disable dynamic form for shell interpreter by default

[jo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jongyoul pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git


The following commit(s) were added to refs/heads/master by this push:
     new 368341c  [ZEPPELIN-5472] Disable dynamic form for shell interpreter by default
368341c is described below

commit 368341c1ae000faa20ef7724f7395a3f8cc13d10
Author: Jeff Zhang <zj...@apache.org>
AuthorDate: Mon Sep 13 17:26:52 2021 +0800

    [ZEPPELIN-5472] Disable dynamic form for shell interpreter by default
    
    ### What is this PR for?
    
    This PR change the form type of shell interpreter from SIMPLE to NATIVE, so that ${var} won't be translated into dynamic form. Because ${var} is often used as variable reference, so we should not confuse user in Zeppelin. If user do want to use dynamic form, they can still use local property to override that. e.g.
    
    ```
    %sh(form=simple)
    
    echo ${name}
    
    ```
    
    ### What type of PR is it?
    [ Improvement ]
    
    ### Todos
    * [ ] - Task
    
    ### What is the Jira issue?
    * https://issues.apache.org/jira/browse/ZEPPELIN-5472
    
    ### How should this be tested?
    * Unit test is added.
    
    ### Screenshots (if appropriate)
    
    ### Questions:
    * Does the licenses files need update? No
    * Is there breaking changes for older versions? No
    * Does this needs documentation? No
    
    Author: Jeff Zhang <zj...@apache.org>
    
    Closes #4218 from zjffdu/ZEPPELIN-5472 and squashes the following commits:
    
    d4f17fb4a [Jeff Zhang] [ZEPPELIN-5472] Disable dynamic form for shell interpreter by default
---
 .../apache/zeppelin/shell/ShellInterpreter.java    |  2 +-
 .../zeppelin/integration/ShellIntegrationTest.java | 91 ++++++++++++++++++++++
 .../integration/ZeppelinClientIntegrationTest.java |  8 +-
 .../apache/zeppelin/rest/NotebookRestApiTest.java  |  2 +-
 4 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java b/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java
index b865a6d..895ed3d 100644
--- a/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java
+++ b/shell/src/main/java/org/apache/zeppelin/shell/ShellInterpreter.java
@@ -203,7 +203,7 @@ public class ShellInterpreter extends KerberosInterpreter {
 
   @Override
   public FormType getFormType() {
-    return FormType.SIMPLE;
+    return FormType.NATIVE;
   }
 
   @Override
diff --git a/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ShellIntegrationTest.java b/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ShellIntegrationTest.java
new file mode 100644
index 0000000..bfeda40
--- /dev/null
+++ b/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ShellIntegrationTest.java
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zeppelin.integration;
+
+import org.apache.zeppelin.conf.ZeppelinConfiguration;
+import org.apache.zeppelin.notebook.Note;
+import org.apache.zeppelin.notebook.Notebook;
+import org.apache.zeppelin.notebook.Paragraph;
+import org.apache.zeppelin.rest.AbstractTestRestApi;
+import org.apache.zeppelin.scheduler.Job;
+import org.apache.zeppelin.user.AuthenticationInfo;
+import org.apache.zeppelin.utils.TestUtils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class ShellIntegrationTest extends AbstractTestRestApi {
+
+
+  @BeforeClass
+  public static void setUp() throws Exception {
+    System.setProperty(ZeppelinConfiguration.ConfVars.ZEPPELIN_HELIUM_REGISTRY.getVarName(),
+            "helium");
+    AbstractTestRestApi.startUp(ShellIntegrationTest.class.getSimpleName());
+  }
+
+  @AfterClass
+  public static void destroy() throws Exception {
+    AbstractTestRestApi.shutDown();
+  }
+
+  @Test
+  public void testBasicShell() throws IOException {
+    Note note = null;
+    try {
+      note = TestUtils.getInstance(Notebook.class).createNote("note1", AuthenticationInfo.ANONYMOUS);
+      Paragraph p = note.addNewParagraph(AuthenticationInfo.ANONYMOUS);
+
+      // test correct shell command
+      p.setText("%sh echo 'hello world'");
+      note.run(p.getId(), true);
+      assertEquals(Job.Status.FINISHED, p.getStatus());
+      assertEquals("hello world\n", p.getReturn().message().get(0).getData());
+
+      // test invalid shell command
+      p.setText("%sh invalid_cmd");
+      note.run(p.getId(), true);
+      assertEquals(Job.Status.ERROR, p.getStatus());
+      assertTrue(p.getReturn().toString(),
+              p.getReturn().message().get(0).getData().contains("command not found"));
+
+      // test shell environment variable
+      p.setText("%sh a='hello world'\n" +
+              "echo ${a}");
+      note.run(p.getId(), true);
+      assertEquals(Job.Status.FINISHED, p.getStatus());
+      assertEquals("hello world\n", p.getReturn().message().get(0).getData());
+
+      // use dynamic form via local property
+      p.setText("%sh(form=simple) a='hello world'\n" +
+              "echo ${a}");
+      note.run(p.getId(), true);
+      assertEquals(Job.Status.FINISHED, p.getStatus());
+      assertEquals(0, p.getReturn().message().size());
+    } finally {
+      if (null != note) {
+        TestUtils.getInstance(Notebook.class).removeNote(note, AuthenticationInfo.ANONYMOUS);
+      }
+    }
+  }
+}
diff --git a/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinClientIntegrationTest.java b/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinClientIntegrationTest.java
index 1ac1106..5cc5a74 100644
--- a/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinClientIntegrationTest.java
+++ b/zeppelin-interpreter-integration/src/test/java/org/apache/zeppelin/integration/ZeppelinClientIntegrationTest.java
@@ -192,7 +192,7 @@ public class ZeppelinClientIntegrationTest extends AbstractTestRestApi {
     assertEquals("hello world\n", paragraphResult.getResults().get(0).getData());
 
     // run paragraph succeed with dynamic forms
-    paragraphId = zeppelinClient.addParagraph(noteId, "run sh", "%sh echo 'hello ${name=abc}'");
+    paragraphId = zeppelinClient.addParagraph(noteId, "run sh", "%sh(form=simple) echo 'hello ${name=abc}'");
     paragraphResult = zeppelinClient.executeParagraph(noteId, paragraphId);
     assertEquals(paragraphId, paragraphResult.getParagraphId());
     assertEquals(paragraphResult.toString(), Status.FINISHED, paragraphResult.getStatus());
@@ -252,7 +252,7 @@ public class ZeppelinClientIntegrationTest extends AbstractTestRestApi {
     assertEquals("hello world\n", paragraphResult.getResults().get(0).getData());
 
     // submit paragraph succeed with dynamic forms
-    paragraphId = zeppelinClient.addParagraph(noteId, "run sh", "%sh echo 'hello ${name=abc}'");
+    paragraphId = zeppelinClient.addParagraph(noteId, "run sh", "%sh(form=simple) echo 'hello ${name=abc}'");
     zeppelinClient.submitParagraph(noteId, paragraphId);
     paragraphResult = zeppelinClient.waitUtilParagraphFinish(noteId, paragraphId, 10 * 1000);
     assertEquals(paragraphId, paragraphResult.getParagraphId());
@@ -325,7 +325,7 @@ public class ZeppelinClientIntegrationTest extends AbstractTestRestApi {
     assertEquals("hello world\n", p0.getResults().get(0).getData());
 
     // update paragraph with dynamic forms
-    zeppelinClient.updateParagraph(noteId, p0Id, "run sh", "%sh echo 'hello ${name=abc}'");
+    zeppelinClient.updateParagraph(noteId, p0Id, "run sh", "%sh(form=simple) echo 'hello ${name=abc}'");
     noteResult = zeppelinClient.executeNote(noteId);
     assertEquals(noteId, noteResult.getNoteId());
     assertEquals(false, noteResult.isRunning());
@@ -396,7 +396,7 @@ public class ZeppelinClientIntegrationTest extends AbstractTestRestApi {
     assertEquals("hello world\n", p0.getResults().get(0).getData());
 
     // update paragraph with dynamic forms
-    zeppelinClient.updateParagraph(noteId, p0Id, "run sh", "%sh sleep 5\necho 'hello ${name=abc}'");
+    zeppelinClient.updateParagraph(noteId, p0Id, "run sh", "%sh(form=simple) sleep 5\necho 'hello ${name=abc}'");
     noteResult = zeppelinClient.submitNote(noteId);
     assertEquals(true, noteResult.isRunning());
     noteResult = zeppelinClient.waitUntilNoteFinished(noteId);
diff --git a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
index 7e57435..9fc51a9 100644
--- a/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
+++ b/zeppelin-server/src/test/java/org/apache/zeppelin/rest/NotebookRestApiTest.java
@@ -466,7 +466,7 @@ public class NotebookRestApiTest extends AbstractTestRestApi {
       Paragraph p1 = note1.addNewParagraph(AuthenticationInfo.ANONYMOUS);
       Paragraph p2 = note1.addNewParagraph(AuthenticationInfo.ANONYMOUS);
       p1.setText("%python name = z.input('name', 'world')\nprint(name)");
-      p2.setText("%sh echo '${name=world}'");
+      p2.setText("%sh(form=simple) echo '${name=world}'");
 
       Map<String, Object> paramsMap = new HashMap<>();
       paramsMap.put("name", "zeppelin");