Skip to content

[ZEPPELIN-1163] Change some parameter name properly in NotebookRestApi - #1187

Closed
zak-hassan wants to merge 4 commits into
apache:masterfrom
zak-hassan:ZEPPELIN-1163
Closed

[ZEPPELIN-1163] Change some parameter name properly in NotebookRestApi#1187
zak-hassan wants to merge 4 commits into
apache:masterfrom
zak-hassan:ZEPPELIN-1163

Conversation

@zak-hassan

@zak-hassanzak-hassan commented Jul 15, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

What is this PR for?

Fix for ZEPPELIN-1163.

What type of PR is it?

Task

Todos

  • - change all web service parameters from notebookId to noteId. Per description provided in jira issue: "Some parameter from rest api are proper to set noteId rather than notebookId"

What is the Jira issue?

How should this be tested?

Execute on the web services or via the ui which is communicating with webservice. Delete note, etc.

Screenshots (if appropriate)

Questions:

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

@AhyoungRyu

AhyoungRyu commented Jul 15, 2016

Copy link
Copy Markdown
Contributor

Is this PR WIP? I think rest-notebook.md file needs to be updated as well.
docs/rest-api/rest-notebook.md

And there are still many notebookId in other files.

@zak-hassan

zak-hassan commented Jul 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Thank you. I'll make some more updates and push fix's to these as well.

@jongyoul

jongyoul commented Jul 15, 2016

Copy link
Copy Markdown
Member

@zmhassan Hi zmhassan. As I mentioned in a JIRA issue, this is in progress by @zjffdu. Could you concede? @zjffdu already contacted me for a few days ago for this issue and I asked for him to work it after I merge ZEPPELIN-1012. I hope he already worked and is waiting for merging ZEPPELIN-1012. Sorry for late reply, and duplicated works.

@zak-hassan

zak-hassan commented Jul 15, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@jongyoul
Sounds good. From what I understand we have to let ZEPPELIN-1012 PR get merged before merging this one, is that correct?

@zjffdu
Is it okay if I complete this work as I've already started? As when I started no one was assigned.

@jongyoul

jongyoul commented Jul 15, 2016

Copy link
Copy Markdown
Member

@zmhassan Thanks for understanding. ZEPPELIN-1012 makes a conflict and this is for beginner thus I don't want for him/her to resolve conflict. @zjffdu Thanks for understanding it.

@zak-hassan

zak-hassan commented Jul 16, 2016

Copy link
Copy Markdown
ContributorAuthor

@AhyoungRyu I've made those fix's pending in the ui and docs as well. Just wondering if its possible to trigger travis CI to run the build again as I know my last commit was WIP

@AhyoungRyu

AhyoungRyu commented Jul 17, 2016

Copy link
Copy Markdown
Contributor

@zmhassan Thanks for your effort! Since ZEPPELIN-1012 was merged, you need to resolve the conflict first. If you need help regarding rebase, please refer to this page. See number 11, 12, 13. It will be helpful to you :)

And most of notebookId that i told were updated but there are more notebookId in jobmanager.controller.js.

@zak-hassan

zak-hassan commented Jul 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Thank you @AhyoungRyu I'm going to fix the rest of jobmanager.controller.js and then rebase.

@zak-hassan

zak-hassan commented Jul 17, 2016

Copy link
Copy Markdown
ContributorAuthor

@AhyoungRyu I've fixed the merge conflict and all changed are now in. Let me know if there is anything else left as I think this task is complete.

jongyoul
jongyoul reviewed Jul 18, 2016
View reviewed changes
Comment threadzeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
@PathParam("paragraphId") String paragraphId) throws IOException {
LOG.info("get paragraph {} {}", notebookId, paragraphId);
public Response getParagraph(@PathParam("noteId") String noteId,
@PathParam("paragraphId") String paragraphId) throws IOException {

@jongyouljongyoulJul 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Vertical align is not recommended. Could you please revert them including the below?

@jongyouljongyoulJul 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zmhassan It's not fixed here.

@bzzbzzJul 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @jongyoul ! Let's make sure this and all other style issues are addressed before merging

@jongyouljongyoulJul 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bzz Yes, I think it's time to enforce style. :-)

@jongyoul

jongyoul commented Jul 18, 2016

Copy link
Copy Markdown
Member

@zmhassan Hi, could you please check the style? Zeppelin doesn't have any strict style for now, but basically follows google's guide. Could you please check it?

@zak-hassan

zak-hassan commented Jul 18, 2016

Copy link
Copy Markdown
ContributorAuthor

Okay all done. I've committed the fix's for this issue. If there is nothing else then I think this task is complete. @jongyoul Thank you so much

jongyoul
jongyoul reviewed Jul 19, 2016
View reviewed changes
Comment threadzeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
throws IOException, CloneNotSupportedException, IllegalArgumentException {
LOG.info("clone notebook by JSON {}", message);
NewNotebookRequest request = gson.fromJson(message, NewNotebookRequest.class);
public Response cloneNote(@PathParam("noteId") String noteId, String message) throws

@jongyouljongyoulJul 19, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not fixed here. throws would be located in a next line.

@jongyoul

jongyoul commented Jul 19, 2016

Copy link
Copy Markdown
Member

@zmhassan You look like that you don't use formatter in your IDE. Could you please change the formatter setting fit in google style and reformat whole of files you changed?

@jongyoul

jongyoul commented Aug 3, 2016

Copy link
Copy Markdown
Member

@zmhassan Can you handle it soon?

@zak-hassan

zak-hassan commented Aug 3, 2016

Copy link
Copy Markdown
ContributorAuthor

@jongyoul Yes. Sorry I got a bit busy.

@jongyoul

jongyoul commented Aug 4, 2016

Copy link
Copy Markdown
Member

@zmhassan Sounds great.

@corneadoug

corneadoug commented Sep 1, 2016

Copy link
Copy Markdown
Contributor

Hi @zmhassan do you think you will have some time to rebase this PR and fix the indent?

@asfgitasfgit closed this in c38a0a0May 9, 2018
asfgit pushed a commit that referenced this pull request May 9, 2018
@jongyoul
Close outdated PRs
b11d355
close#83close#86close#125close#133close#139close#146close#193close#203close#246close#262close#264close#273close#291close#299close#320close#347close#389close#413close#423close#543close#560close#658close#670close#728close#765close#777close#782close#783close#812close#822close#841close#843close#878close#884close#918close#989close#1076close#1135close#1187close#1231close#1304close#1316close#1361close#1385close#1390close#1414close#1422close#1425close#1447close#1458close#1466close#1485close#1492close#1495close#1497close#1536close#1545close#1561close#1577close#1600close#1603close#1678close#1695close#1739close#1748close#1765close#1767close#1776close#1783close#1799
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zak-hassan@AhyoungRyu@jongyoul@corneadoug@bzz