Skip to content

[ZEPPELIN-484] A small utility to help review the pullrequest.#513

Closed
Leemoonsoo wants to merge 4 commits into
apache:masterfrom
Leemoonsoo:ZEPPELIN-484
Closed

[ZEPPELIN-484] A small utility to help review the pullrequest.#513
Leemoonsoo wants to merge 4 commits into
apache:masterfrom
Leemoonsoo:ZEPPELIN-484

Conversation

@Leemoonsoo

Copy link
Copy Markdown
Member

This PR address https://issues.apache.org/jira/browse/ZEPPELIN-484.

We've got discussions recently about impasse of pullrequest and speeding up of accepting pullrequest.
This small utility can be used when reviewer want to test the pullrequest.

Usage is

dev/test.py [#PR]

for example

dev/test.py 500

will create local branch pr500 and add remote repository of contribution, fetch and merge it for the test.

This script will help reduce small bit of time of reviewing.

@r-kamath

r-kamath commented Dec 4, 2015

Copy link
Copy Markdown
Member

@Leemoonsoo I do curl https://patch-diff.githubusercontent.com/raw/apache/incubator-zeppelin/pull/<PR#>.patch | git apply in master, review it and reset!

@prabhjyotsingh

Copy link
Copy Markdown
Contributor

@r-kamath solution looks simpler, both are hacky though 😃

@jongyoul

jongyoul commented Dec 5, 2015

Copy link
Copy Markdown
Member

I've tested. Looks good. It's enough to me to test pr. Thanks.

@corneadoug

Copy link
Copy Markdown
Contributor

Should we also include some:
git clean -dxf and then launch build then?

@bzz

bzz commented Dec 8, 2015

Copy link
Copy Markdown
Member

Looks great to me.

Another way to do the same is a small hub github/hub that allows you to do, apart from many other useful things, PR checkouts like

hub checkout https://github.com/apache/incubator-zeppelin/pull/513

It takes care of setting up a remote (so you can always pull from it later) and checking out a PR branch to your local repo.

@bzz

bzz commented Dec 8, 2015

Copy link
Copy Markdown
Member

@Leemoonsoo If we want to go further with this tool, as we already have ./dev/merge_zeppelin_pr.py may be we could name it something like ./dev/test_zeppelin_pr.py?

I feel that this, as well as a couple of lines of text comments in the header after the licence boilerplate, will give a better idea of it's purpose for people who will just notice it in the filesystem, what do you think?

@Leemoonsoo

Copy link
Copy Markdown
Member Author

Thanks all the feedbacks and alternative solutions.

I have changed script name from test.py to test_zeppelin_pr.py and add more comment.
Adding git clean -dxf will remove untracked files. I think it's better leave it up to developers.

@bzz

bzz commented Dec 10, 2015

Copy link
Copy Markdown
Member

Looks great to me

@Leemoonsoo

Copy link
Copy Markdown
Member Author

merging into master

@asfgit asfgit closed this in 6b69ed4 Dec 12, 2015
asfgit pushed a commit that referenced this pull request Jan 21, 2016
### What is this PR for?
Zeppelin has already provided a useful utility for reviewing PR ( #513 ). So I added a **Testing a Pull Request** section to `CONTRIBUTING.md` for whom to review and test PR quickly.

### What type of PR is it?
Documentation

### Todos
* [x] - Add a **Testing a Pull Request** section to CONTRIBUTING.md

### Is there a relevant Jira issue?
[ZEPPELIN-484](https://issues.apache.org/jira/browse/ZEPPELIN-484) and PR #513

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Ryu Ah young <fbdkdud93@hanmail.net>

Closes #656 from AhyoungRyu/add_pr_review_information and squashes the following commits:

5f370d5 [Ryu Ah young] Fix some grammar errors
3781550 [Ryu Ah young] Delete 'like'
9ced186 [Ryu Ah young] Add some information about how we can review and test PR
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.

6 participants