Skip to content

Conversation

@chilang
Copy link
Contributor

@chilang chilang commented Sep 29, 2017

What is this PR for?

Support updating paragraph text/title via REST API
https://issues.apache.org/jira/browse/ZEPPELIN-1897

What type of PR is it?

Improvement

Todos

What is the Jira issue?

[ZEPPELIN-1897]

How should this be tested?

  1. Create a note and paragraph
  2. Hit REST API to update the paragraph, e.g. curl -X PUT http://localhost:8080/api/notebook/NOTE_ID/paragraph/PARAGRAPH_ID -d '{"text": "some updated text"}'

Screenshots (if appropriate)

update paragraph api

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes, doc update included in the change.

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

could you update the PR title to say "update paragraph title"

"status": "OK",
"message": "",
"body": {
"title": "Hellow world",
Copy link
Member

Choose a reason for hiding this comment

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

this is the text of the paragraph? do we need to return it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixcheung Good point. Don't see the need at this point so I've removed the response's body. Docs updated.

@chilang chilang changed the title ZEPPELIN-1897 REST API - Update paragraph via API ZEPPELIN-1897 REST API - Update paragraph text/title via API Oct 1, 2017
@chilang
Copy link
Contributor Author

chilang commented Oct 1, 2017

@felixcheung I've updated the the PR title to be more clear

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

this LGTM. since this is updating an existing paragraph there might be other implications that I'm not aware of, I'd like others to review as well

post.releaseConnection();



Copy link
Member

Choose a reason for hiding this comment

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

please remove extra newlines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@felixcheung
Copy link
Member

also, there are some test failures, could you take a look?

@chilang
Copy link
Contributor Author

chilang commented Oct 2, 2017

@felixcheung The build seems to be very flaky.
In any case they're passing now , except for the web e2e tests in the build #2 (which is failing consistently on master so I think it's unrelated to the change)

@felixcheung
Copy link
Member

ok, agreed that seems unrelated.

btw, it would be easier for reviewer to track changes if you do not force push

any comment on this PR? I'd wait for a couple of days.

@asfgit asfgit closed this in 3eeb228 Oct 9, 2017
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.

2 participants