[GitHub] zeppelin pull request #2268: [ZEPPELIN-2403] interpreter property widgets

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin pull request #2268: [ZEPPELIN-2403] interpreter property widgets

herval
GitHub user tinkoff-dwh opened a pull request:

    https://github.com/apache/zeppelin/pull/2268

    [ZEPPELIN-2403] interpreter property widgets

    ### What is this PR for?
    I spoiled the previous PR #2251
   
    Added widgets (string, text, url, password, url, checkbox) to properties of interpreters. Those are widgets for properties customization. Properties must have the ability to customize the display (for example password).
   
   
    ### What type of PR is it?
    Feature
   
    ### What is the Jira issue?
    https://issues.apache.org/jira/browse/ZEPPELIN-2403
   
    ### How should this be tested?
    - remove conf/interpreter.json
    - Try new form (create, edit) of interpreter settings
   
    ### Screenshots (if appropriate)
    edit
    ![edit](https://cloud.githubusercontent.com/assets/25951039/25130228/e2a28060-245a-11e7-895a-d7c1571f885f.png)
   
    view
    ![view](https://cloud.githubusercontent.com/assets/25951039/25130227/e2a10906-245a-11e7-9ea3-0bd070219f42.png)
   
    ### Questions:
    * Does the licenses files need update? no
    * Is there breaking changes for older versions? no
    * Does this needs documentation? no


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

    $ git pull https://github.com/tinkoff-dwh/zeppelin ZEPPELIN-2403

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

    https://github.com/apache/zeppelin/pull/2268.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 #2268
   
----
commit 45f5f6276d731084b3873cb1885f684ab1449e68
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-14T07:44:29Z

    ZEPPELIN-2403 added interpreter property types

commit 12499ae1ef7d329c7f3e74b4298031cfe4b9c490
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-17T08:01:39Z

    Merge remote-tracking branch 'upstream/master' into ZEPPELIN-2403
   
    # Conflicts:
    # docs/install/upgrade.md

commit 14353b1200443582b1fceafabce0e82cdf83a054
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-17T08:02:01Z

    Merge remote-tracking branch 'upstream/master' into ZEPPELIN-2403

commit dd5d6c809aeb2bdb72357a072954cf4a9c882487
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-17T08:38:59Z

    ZEPPELIN-2403 did properties immutable, added new type 'checkbox'

commit 4f271d9b03a1748d83fefdf290abbba68d3cd280
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-18T12:16:00Z

    ZEPPELIN-2403  rename to widget  added new widgets  string,  number,  url

commit fd8d27810cfc004216c8ea61ea7fe5d06aa9bde2
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-20T10:42:05Z

    Merge remote-tracking branch 'origin/master' into ZEPPELIN-2403_backup
   
    # Conflicts:
    # zeppelin-web/src/app/interpreter/interpreter.controller.js
    # zeppelin-web/src/index.js

commit a495137ffccd7d88adb9593ada3cff2cb6e10698
Author: Tinkoff DWH <[hidden email]>
Date:   2017-04-20T10:49:24Z

    ZEPPELIN-2403 eslint fix

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    1. number, string and url can be generalized as `text`. For url, we can use `ng-html` with angular's `$sse` service to draw `<a>..</a>` element
    2. if `text` widget means `textarea` DOM, can we change it to `textarea` instead of?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    @1ambda
    I think it's too complicate code, for example generate list (combobox) of widgets. How will the selection of the type of the password, second combobox?. I see no reason to generalize number, string and url.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    One example of separating widget with value types.
   
    ```
    // https://github.com/apache/zeppelin/blob/master/zeppelin-web/src/app/tabledata/advanced-transformation-util.test.js#L19-L25
   
    const MockParameter = {
      'floatParam': { valueType: 'float', defaultValue: 10, description: '', },
      'intParam': { valueType: 'int', defaultValue: 50, description: '', },
      'jsonParam': { valueType: 'JSON', defaultValue: '', description: '', widget: 'textarea', },
      'stringParam1': { valueType: 'string', defaultValue: '', description: '', },
      'stringParam2': { valueType: 'string', defaultValue: '', description: '', widget: 'input', },
      'boolParam': { valueType: 'boolean', defaultValue: false, description: '', widget: 'checkbox', },
      'optionParam': { valueType: 'string', defaultValue: 'line', description: '', widget: 'option', optionValues: [ 'line', 'smoothedLine', ], },
    }
    ```
   
    1. In case of password, value type will be decided by its definition. If it's number, we can validate it as number. (string as well)
   
    2. In case of combobox, type will be boolean.
   
    3. We can parse values based on their type like <a href="https://github.com/apache/zeppelin/blob/master/zeppelin-web/src/app/tabledata/advanced-transformation-util.js#L85-#L117">https://github.com/apache/zeppelin/blob/master/zeppelin-web/src/app/tabledata/advanced-transformation-util.js#L85-#L117
   
    4. I think it's not complicated. IMO, it's the part we should design carefully since it's hard to change (backward compatibility)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    CI red (checkstyle errors in all modules)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    Ready to review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    Let me review and comment soon for this awesome improvement.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    Sorry for the late review.
   
    seems that it doesn't work with existing settings. Even can't start Zeppelin instance.
   
    ```
    Caused by: java.lang.ClassCastException: java.lang.String cannot be cast to com.google.gson.internal.StringMap
            at org.apache.zeppelin.interpreter.InterpreterSettingManager.loadFromFile(InterpreterSettingManager.java:174)
            at org.apache.zeppelin.interpreter.InterpreterSettingManager.init(InterpreterSettingManager.java:345)
            at org.apache.zeppelin.interpreter.InterpreterSettingManager.<init>(InterpreterSettingManager.java:150)
            at org.apache.zeppelin.server.ZeppelinServer.<init>(ZeppelinServer.java:149)
            ... 29 more
    ```
   
    - It would be nicer to provide migration function like https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Notebook.java#L418
    - or use the default type / widget if a field doens't have (e.g `string`, `input`)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    @1ambda
    i wrote about migration in upgrade.md. I'll see Notebook.java if the documentation is not enough


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    As far as I Know, Zeppelin supports backward compatibility even for interpreter settings.
   
    - I think we need some migration code w/ documentation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    This PR adds two new field 'widget' and 'type' to each properties.
   
    'widget' : input, checkbox, textarea, password
    'type' : number, string, boolean, url, password
   
    How about just add single field 'type' with possible values "string"(input), "number", "url", "password", "checkbox", "text"(textarea) ?
   
    @1ambda mentioned about backward compatibility. It'll help users who upgrade Zeppelin, if this PR reads `conf/interpreter.json` generated by previous version.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    @Leemoonsoo
    the separation discussed above in the comments


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    CI red https://travis-ci.org/tinkoff-dwh/zeppelin/builds/231079773
   
    another tests
      SecurityRestApiTest.testGetUserList:69 » JsonSyntax java.lang.IllegalStateExce...
   
      SecurityRestApiTest.testTicket:55 » JsonSyntax java.lang.IllegalStateException...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    ready to review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    > the separation discussed above in the comments
   
    maybe i missed, @tinkoff-dwh could you point out the comment?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    @Leemoonsoo
    first three comments
   
    ...
    I think it's not complicated. IMO, it's the part we should design carefully since it's hard to change (backward compatibility)
    ...
    widget means UI, and type defines their actual value types. Sometime they might be mixed together, but usually not.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    If i list all combinations of widget-type in this PR,
   
    | widget  | type |
    | ------------- | ------------- |
    | input  | **string**  |
    | input | **number**  |
    | **textarea** | string |
    | **password** | password |
    | **checkbox** | boolean |
    | input | **url** |
   
    As we can see, each combination can be uniquely identified by either widget or type. (bold font)
    Unless there's big plan in the future add a lot of combinations of those, i think just keeping one field 'type' is enough to identify all possible combinations.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    looks good. initially, the idea with the type well, as the type indicated the type of the stored value (widget is pointing to), but **password** all broke)
   
    @1ambda
    what do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets

herval
In reply to this post by herval
Github user 1ambda commented on the issue:

    https://github.com/apache/zeppelin/pull/2268
 
    IMO, It's ok if we are not going to add other types like `JSON` which can be `type` and used for other widgets (e.g `textarea`, ...)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...