[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/2268 merge if no more discussion. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user tinkoff-dwh commented on the issue: https://github.com/apache/zeppelin/pull/2268 @1ambda done --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/2268 @tinkoff-dwh Please resolve conflict. Usually, we merge after 1 day when someone left LGTM, but this time, conflict occurred before 1 day. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/2268 LGTM, merge if no more discussion. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user tinkoff-dwh commented on the issue: https://github.com/apache/zeppelin/pull/2268 1,2 done @1ambda thx for 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/2268 1. Please resolve the conflict. 2. Left a minor comment please check that. 3. Tested locally and works well with 6 newly added types. 4. Backward compatibility will be handed by the `convertFlatPropertiesToPropertiesWithWidgets` func (check interpreter config and convert old format to new format) After 1, 2 are resolved, merge it if no more discussion. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user 1ambda commented on the issue: https://github.com/apache/zeppelin/pull/2268 I am not a big fan of this approach. But reviewers discussed and I partially agree in terms of complexity. I will check few things since it was quite old when I last reviewed this, Then if it's good will merge if no more discussion. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user tinkoff-dwh commented on the issue: https://github.com/apache/zeppelin/pull/2268 @Leemoonsoo @1ambda maybe merge? 2 month :) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user tinkoff-dwh commented on the issue: https://github.com/apache/zeppelin/pull/2268 @Leemoonsoo ? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
Github user tinkoff-dwh commented on the issue: https://github.com/apache/zeppelin/pull/2268 @Leemoonsoo done --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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.(InterpreterSettingManager.java:150) at org.apache.zeppelin.server.ZeppelinServer.(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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zeppelin issue #2268: [ZEPPELIN-2403] interpreter property widgets
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 `..` 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
