Skip to content

Conversation

@khalidhuseynov
Copy link
Member

What is this PR for?

This PR improves the front-end experience to use git versioning on notebooks.
Any community feedback is welcome.

What type of PR is it?

Improvement

Todos

  • - front-end and back-end changes
  • - add checkpoint interface
  • - error propagation to front-end in a separate PR
  • - tests
  • - UI feedback?

Is there a relevant Jira issue?

ZEPPELIN-540

How should this be tested?

  1. Uncomment GitNotebookRepo as a storage in /conf/zeppelin-site.xml
  2. Start Zeppelin
  3. Make changes to some notebook
  4. Use git menu to commit
  5. To check whether successfully committed, do git log in /notebooks folder

Screenshots (if appropriate)

zeppelin commit notebook

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@Leemoonsoo
Copy link
Member

@khalidhuseynov Thanks for the contribution. I think the approach make sense, but the implementation need to be more generalized. We must not assume that user will use git notebook repo.

How about add one more interface to NotebookRepo?

public void checkpoint(String noteId, String checkPointName);

and let all notebookrepo implement this interface. This method can be invoked with proposed GUI.
Each different notebook storage can implement checkpoint() for their own purpose. GitNotebookRepo may use this method for making commit, for example.

What do you think?

@khalidhuseynov
Copy link
Member Author

@Leemoonsoo yes that makes it more flexible and makes sense, I'll address it soon.

@khalidhuseynov khalidhuseynov changed the title Zeppelin-540: Notebook versioning control (git commit) from frontend notebook menu Zeppelin-540: [WIP] Notebook versioning control (git commit) from frontend notebook menu Dec 29, 2015
@maaark11
Copy link

guys you have some communication channel, I enjoyed the project and want to help

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just setMessage(commitMessage) as a commit message? is there special reason adding "Updated " + pattern in front?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! Actually as I've checked now, pattern from setMessage isn't used anywhere else. addressed it in b0fa219

@Leemoonsoo
Copy link
Member

@maaark11 Thanks for the interest : )
Mailing list here http://zeppelin.incubator.apache.org/community.html are official communication channel. You can subscribe and post message to the mailing list for any question, suggestion, feedback, discussion, etc.

Conflicts:
	zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
@khalidhuseynov khalidhuseynov force-pushed the feat/git-commit-frontend-access branch from 31906bd to 339c9fe Compare January 19, 2016 02:30
@khalidhuseynov khalidhuseynov force-pushed the feat/git-commit-frontend-access branch from 99c7122 to b0fa219 Compare January 19, 2016 04:52
@khalidhuseynov
Copy link
Member Author

tests were added, ready for review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use just 'message' or 'commitMessage' instead of 'gitCommitMessage' for the field name?

@khalidhuseynov
Copy link
Member Author

@Leemoonsoo yes exactly! I was going to change git related things, but also wanted to hear community on UI feedback. Thanks for the feedback, will be addressed!

@khalidhuseynov khalidhuseynov changed the title Zeppelin-540: [WIP] Notebook versioning control (git commit) from frontend notebook menu Zeppelin-540: [WIP] Notebook versioning control, commit from frontend notebook menu Jan 20, 2016
Khalid Huseynov added 4 commits January 24, 2016 02:27
Conflicts:
	zeppelin-server/src/main/java/org/apache/zeppelin/socket/Message.java
	zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
@khalidhuseynov
Copy link
Member Author

The code and screenshot has been updated. Currently the icon for version control was changed from git to file-code icon. However that's subject to change depending on the feedback. Other possible icons could be anchor, fork or any other suggested by community.

@khalidhuseynov khalidhuseynov changed the title Zeppelin-540: [WIP] Notebook versioning control, commit from frontend notebook menu Zeppelin-540: Notebook versioning control, commit from frontend notebook menu Jan 24, 2016
Conflicts:
	zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
@khalidhuseynov khalidhuseynov force-pushed the feat/git-commit-frontend-access branch from b5345df to 64111e0 Compare February 2, 2016 09:26
}
git = new Git(localRepo);
maybeAddAndCommit(".");
checkpoint(".", "initialization commit");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shouldn't be executed when Git repo already exists. Isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, that's more friendly. Actually I changed it not to commit even at first start when initializing Git repo. So on first start all the notebooks will be unstaged, and user will explicitly commit whenever needed.

@khalidhuseynov khalidhuseynov force-pushed the feat/git-commit-frontend-access branch 3 times, most recently from 3312ece to 3f510d1 Compare February 8, 2016 17:58
@khalidhuseynov khalidhuseynov force-pushed the feat/git-commit-frontend-access branch from 3f510d1 to 4f04af0 Compare February 8, 2016 20:01
@Leemoonsoo
Copy link
Member

Thanks for addressing comment.
It's great improvement.
LGTM

@Leemoonsoo
Copy link
Member

Merge if there're no more discussions

@asfgit asfgit closed this in 7a58d46 Feb 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants