[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-12 Thread prabhjyotsingh 
Github user prabhjyotsingh commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
Thanks for the review will merge this if no more discussion.


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-12 Thread zjffdu 
Github user zjffdu commented on the issue:

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


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-07 Thread prabhjyotsingh 
Github user prabhjyotsingh commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
Shall I merge this, if no more discussion?


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-06 Thread prabhjyotsingh 
Github user prabhjyotsingh commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
@zjffdu have made the suggested changes, waiting for CI, does this looks 
good?


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-05 Thread zjffdu 
Github user zjffdu commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
It sounds a little redundant to ask user to both implement these version 
related method and `isRevisionSupported`. How about creating a sub-interface of 
`NotebookRepo` which support version control ? And if NotebookRepo using is the 
implementation of this sub-interface, we show version control on UI


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-01 Thread prabhjyotsingh 
Github user prabhjyotsingh commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
@felixcheung @zjffdu  can you please help review this?


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-01 Thread prabhjyotsingh 
Github user prabhjyotsingh commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
Yes, I believe that is exactly what I'm trying to do over here, for example 
in S3NotebookRepo, since none of checkpoint, get, revisionHistory, or 
setNoteRevision is implemented 
(https://github.com/apache/zeppelin/blob/master/zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/S3NotebookRepo.java#L292)
 hence for the same the method isRevisionSupported returns false. Let me know 
if I'm missing out on a case.


---

[GitHub] zeppelin issue #2757: [ZEPPELIN-3198] UI should not show Version/GIT Control...

2018-02-01 Thread jhonderson 
Github user jhonderson commented on the issue:

https://github.com/apache/zeppelin/pull/2757
  
The methods for the versioning of the notes are defined at interface level 
in NotebookRepo.java:

 - Revision checkpoint(String noteId, String checkpointMsg, 
AuthenticationInfo)
 - Note get(String noteId, String revId, AuthenticationInfo)
 - List revisionHistory(String noteId, AuthenticationInfo)
 - Note setNoteRevision(String noteId, String revId, AuthenticationInfo)

They could be implemented by a non-git repository, for example a repository 
that saves the notes in S3 and its versioning information in a database or 
something like that. So i guess a better solution would be a method in 
NotebookRepo.java that indicates if the implementation supports versioning 
(maybe doesSupportVersioning()); and use that method to show or hide the 
versioning feature.

What do you think?.


---