Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: tools
    • Labels:
      None

      Description

      Created a new patch review tool that will integrate JIRA and reviewboard - https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool

      1. KAFKA-1053_2013-09-15_20:28:01.patch
        4 kB
        Neha Narkhede
      2. KAFKA-1053_2013-09-16_14:40:15.patch
        4 kB
        Neha Narkhede
      3. KAFKA-1053-2013-09-15_09:40:04.patch
        4 kB
        Neha Narkhede
      4. KAFKA-1053-followup.patch
        4 kB
        Neha Narkhede
      5. KAFKA-1053-followup2.patch
        4 kB
        Neha Narkhede
      6. KAFKA-1053-v1.patch
        4 kB
        Neha Narkhede
      7. KAFKA-1053-v1.patch
        4 kB
        Neha Narkhede
      8. KAFKA-1053-v1.patch
        3 kB
        Neha Narkhede
      9. KAFKA-1053-v2.patch
        4 kB
        Neha Narkhede
      10. KAFKA-1053-v3.patch
        4 kB
        Neha Narkhede

        Activity

        Neha Narkhede created issue -
        Hide
        Tejas Patil added a comment -

        (1) In [0], "Setup" -> hyperlinks on steps 1 and 2 loop to the same webpage.

        (2) I don't have much idea about the right place where the ".reviewboardrc" file should be, but it would be a good idea to commit it in the codebase like [1]. Also, add it to .gitignore (like [2]).

        (3) How about adding "kafka-rb.py" to kafka codebase ? With that maybe there won't be any need for JIRA_CMDLINE_HOME.

        (4) In kafka-rb.py:

        > popt.add_argument('s', '-summary', action='store', dest='summary', required=False, help='Summary for the reviewboard')
        > popt.add_argument('d', '-description', action='store', dest='description', required=False

        I am wondering if someone doesn't provide a summary and as its an optional param, the script won't complain. Eventually, RB dashboard would end up having a bunch of tickets with no summary or title.

        (6) > print 'Creating reviewboard'
        Could this message sound good: "Generating a new review board ticket" ?

        (7) Is there a way to specify the "Testing Done" text of RB through this script ?

        [0] : https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool
        [1] : https://issues.apache.org/jira/browse/GIRAPH-331
        [2] : https://issues.apache.org/jira/browse/TAJO-69

        Show
        Tejas Patil added a comment - (1) In [0] , "Setup" -> hyperlinks on steps 1 and 2 loop to the same webpage. (2) I don't have much idea about the right place where the ".reviewboardrc" file should be, but it would be a good idea to commit it in the codebase like [1] . Also, add it to .gitignore (like [2] ). (3) How about adding "kafka-rb.py" to kafka codebase ? With that maybe there won't be any need for JIRA_CMDLINE_HOME. (4) In kafka-rb.py: > popt.add_argument(' s', ' -summary', action='store', dest='summary', required=False, help='Summary for the reviewboard') > popt.add_argument(' d', ' -description', action='store', dest='description', required=False I am wondering if someone doesn't provide a summary and as its an optional param, the script won't complain. Eventually, RB dashboard would end up having a bunch of tickets with no summary or title. (6) > print 'Creating reviewboard' Could this message sound good: "Generating a new review board ticket" ? (7) Is there a way to specify the "Testing Done" text of RB through this script ? [0] : https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool [1] : https://issues.apache.org/jira/browse/GIRAPH-331 [2] : https://issues.apache.org/jira/browse/TAJO-69
        Neha Narkhede made changes -
        Field Original Value New Value
        Attachment KAFKA-1053-v1.patch [ 12602649 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602650 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v2.patch [ 12602652 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602653 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v2.patch [ 12602652 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602653 ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602649 ]
        Neha Narkhede made changes -
        Comment [ Updated reviewboard ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Neha Narkhede added a comment -

        Thanks for the quick review Tejas

        1. I believe Guozhang fixed this
        2. Updated the reviewboard to include the .reviewboardrc file for checkin to the codebase
        3. Added kafka-patch-review.py to the reviewboard for checkin to the codebase
        4. Added default summary "Patch for KAFKA-..."
        6. There are only 2 review board tasks a) Creating a new reviewboard b) Updating an existing reviewboard. Hopefully "Creating a new reviewboard" explains that better
        7. Added a "--testing-done" option to the script

        Show
        Neha Narkhede added a comment - Thanks for the quick review Tejas 1. I believe Guozhang fixed this 2. Updated the reviewboard to include the .reviewboardrc file for checkin to the codebase 3. Added kafka-patch-review.py to the reviewboard for checkin to the codebase 4. Added default summary "Patch for KAFKA-..." 6. There are only 2 review board tasks a) Creating a new reviewboard b) Updating an existing reviewboard. Hopefully "Creating a new reviewboard" explains that better 7. Added a "--testing-done" option to the script
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602662 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v1.patch [ 12602827 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Neha Narkhede added a comment -

        Any feedback from other committers Jun Rao, Joel Koshy ?

        Show
        Neha Narkhede added a comment - Any feedback from other committers Jun Rao , Joel Koshy ?
        Neha Narkhede made changes -
        Comment [ Test comment ]
        Hide
        Joel Koshy added a comment -

        I'll take a look today - would like to try it out as well

        Show
        Joel Koshy added a comment - I'll take a look today - would like to try it out as well
        Neha Narkhede made changes -
        Comment [ Test comment ]
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v2.patch [ 12602880 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Neha Narkhede made changes -
        Attachment KAFKA-1053-v3.patch [ 12602882 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Neha Narkhede made changes -
        Attachment KAFKA-1053-followup.patch [ 12603133 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Neha Narkhede made changes -
        Attachment KAFKA-1053-followup2.patch [ 12603160 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Joel Koshy added a comment -

        Ran into this while following the instructions. May be some python version
        conflict but throwing it out there in case someone encountered this and
        worked around it:

        [1709][jkoshy@jkoshy-ld:~]$ sudo easy_install -U setuptools
        Traceback (most recent call last):
        File "/usr/bin/easy_install", line 9, in <module>
        load_entry_point('distribute', 'console_scripts', 'easy_install')()
        File "/usr/lib/python2.6/site-packages/setuptools-1.1.5-py2.6.egg/pkg_resources.py", line 357, in load_entry_point
        return get_distribution(dist).load_entry_point(group, name)
        File "/usr/lib/python2.6/site-packages/setuptools-1.1.5-py2.6.egg/pkg_resources.py", line 2393, in load_entry_point
        raise ImportError("Entry point %r not found" % ((group,name),))
        ImportError: Entry point ('console_scripts', 'easy_install') not found

        Show
        Joel Koshy added a comment - Ran into this while following the instructions. May be some python version conflict but throwing it out there in case someone encountered this and worked around it: [1709] [jkoshy@jkoshy-ld:~] $ sudo easy_install -U setuptools Traceback (most recent call last): File "/usr/bin/easy_install", line 9, in <module> load_entry_point('distribute', 'console_scripts', 'easy_install')() File "/usr/lib/python2.6/site-packages/setuptools-1.1.5-py2.6.egg/pkg_resources.py", line 357, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/usr/lib/python2.6/site-packages/setuptools-1.1.5-py2.6.egg/pkg_resources.py", line 2393, in load_entry_point raise ImportError("Entry point %r not found" % ((group,name),)) ImportError: Entry point ('console_scripts', 'easy_install') not found
        Hide
        Joel Koshy added a comment -

        Probably because I did both yum install python-setuptools and easy_install -U setuptools

        Show
        Joel Koshy added a comment - Probably because I did both yum install python-setuptools and easy_install -U setuptools
        Hide
        Neha Narkhede added a comment -

        Ya, I think the post review page is a bit confusing. I updated the wiki with the precise installation instructions for review board - https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool#Kafkapatchreviewtool-1.Installthepostreviewtool

        Show
        Neha Narkhede added a comment - Ya, I think the post review page is a bit confusing. I updated the wiki with the precise installation instructions for review board - https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool#Kafkapatchreviewtool-1.Installthepostreviewtool
        Neha Narkhede made changes -
        Attachment KAFKA-1053-2013-09-15_09:40:04.patch [ 12603239 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Neha Narkhede added a comment -

        Thanks for the review comments, Swapnil Ghike. Were you able to setup the tool correctly and use it to upload a patch and rb?

        Show
        Neha Narkhede added a comment - Thanks for the review comments, Swapnil Ghike . Were you able to setup the tool correctly and use it to upload a patch and rb?
        Hide
        Neha Narkhede added a comment -

        I think this tool will be easier to use if it is checked in. That will also simplify the wiki. Can I get a +1 from one of the committers?

        Show
        Neha Narkhede added a comment - I think this tool will be easier to use if it is checked in. That will also simplify the wiki. Can I get a +1 from one of the committers?
        Hide
        Swapnil Ghike added a comment - - edited

        Hmm, tried setting up the tool according to the instruction for RHEL. Ran into this:
        ~/kafka/kafka$ python kafka-patch-review.py --help
        Traceback (most recent call last):
        File "kafka-patch-review.py", line 3, in <module>
        import argparse
        ImportError: No module named argparse

        Does the easy_install think work only on Mac? (jira-python and RBTools are installed using easy_install)?

        On the Mac, I got this:
        ~/kafka-local/kafka$ echo $JIRA_CMDLINE_HOME
        .
        ~/kafka-local/kafka$ python kafka-patch-review.py -b 0.8 -j KAFKA-1003 -db
        Jira Home= .
        git diff 0.8 > KAFKA-1003.patch
        Creating diff against 0.8 and uploading patch to JIRA KAFKA-1003
        Creating a new reviewboard
        post-review --publish --tracking-branch 0.8 --target-groups=kafka --bugs-closed=KAFKA-1003 --summary "Patch for KAFKA-1003"
        There don't seem to be any diffs!

        rb url=

        If you take a look at KAFKA-1003, it has appended my diffs, it just did not crete a review board. I guess this is expected.

        Show
        Swapnil Ghike added a comment - - edited Hmm, tried setting up the tool according to the instruction for RHEL. Ran into this: ~/kafka/kafka$ python kafka-patch-review.py --help Traceback (most recent call last): File "kafka-patch-review.py", line 3, in <module> import argparse ImportError: No module named argparse Does the easy_install think work only on Mac? (jira-python and RBTools are installed using easy_install)? On the Mac, I got this: ~/kafka-local/kafka$ echo $JIRA_CMDLINE_HOME . ~/kafka-local/kafka$ python kafka-patch-review.py -b 0.8 -j KAFKA-1003 -db Jira Home= . git diff 0.8 > KAFKA-1003 .patch Creating diff against 0.8 and uploading patch to JIRA KAFKA-1003 Creating a new reviewboard post-review --publish --tracking-branch 0.8 --target-groups=kafka --bugs-closed= KAFKA-1003 --summary "Patch for KAFKA-1003 " There don't seem to be any diffs! rb url= If you take a look at KAFKA-1003 , it has appended my diffs, it just did not crete a review board. I guess this is expected.
        Hide
        Neha Narkhede added a comment -

        Thanks for giving it a spin, though I'm not sure you are using the latest version of the tool.

        1. For the argparse error, I'm not too sure if it is because argparse requires Python 2.7.x ? Could you try upgrading Python to 2.7.x and see if it works?
        2. To upload a patch using the tool, the code must be committed to the local branch else the diff is empty. But I think that points to a potential improvement to the tool. If the diff is empty, it can skip uploading the patch and creating the rb.

        Show
        Neha Narkhede added a comment - Thanks for giving it a spin, though I'm not sure you are using the latest version of the tool. 1. For the argparse error, I'm not too sure if it is because argparse requires Python 2.7.x ? Could you try upgrading Python to 2.7.x and see if it works? 2. To upload a patch using the tool, the code must be committed to the local branch else the diff is empty. But I think that points to a potential improvement to the tool. If the diff is empty, it can skip uploading the patch and creating the rb.
        Hide
        Swapnil Ghike added a comment -

        1. RHEL machine is on python 2.7.2. Maybe the libraries are not in the standard place or something.
        2. Made a local commit, tried again, same error. I am using the latest diff (the one with the timestamps). Is JIRA_CMDLINE_HOME pointing to a wrong directory? I set it to .

        Show
        Swapnil Ghike added a comment - 1. RHEL machine is on python 2.7.2. Maybe the libraries are not in the standard place or something. 2. Made a local commit, tried again, same error. I am using the latest diff (the one with the timestamps). Is JIRA_CMDLINE_HOME pointing to a wrong directory? I set it to .
        Neha Narkhede made changes -
        Attachment KAFKA-1053_2013-09-15_20:28:01.patch [ 12603269 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Neha Narkhede added a comment -

        Updated the Setup section of the wiki with instructions on installing argparse - https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool#Kafkapatchreviewtool-1.Setup
        Also, JIRA_CMDLINE_HOME is defunct.
        I improved the error reporting of the tool to handle empty diffs. Could you give the latest version a try?

        Show
        Neha Narkhede added a comment - Updated the Setup section of the wiki with instructions on installing argparse - https://cwiki.apache.org/confluence/display/KAFKA/Kafka+patch+review+tool#Kafkapatchreviewtool-1.Setup Also, JIRA_CMDLINE_HOME is defunct. I improved the error reporting of the tool to handle empty diffs. Could you give the latest version a try?
        Hide
        Swapnil Ghike added a comment -

        After installing arg_parse and using origin/0.8 instead of 0.8 as the branch name, it worked like a charm!

        Show
        Swapnil Ghike added a comment - After installing arg_parse and using origin/0.8 instead of 0.8 as the branch name, it worked like a charm!
        Hide
        Neha Narkhede added a comment -

        Cool! Swapnil, could you help me add a FAQ to this wiki. It will be great if you could list the issues you ran into with the error messages.

        Show
        Neha Narkhede added a comment - Cool! Swapnil, could you help me add a FAQ to this wiki. It will be great if you could list the issues you ran into with the error messages.
        Hide
        Swapnil Ghike added a comment - - edited

        Saw new issue on RHEL when I tried 'python kafka-patch-review.py -b origin/trunk -j KAFKA-42 -r 14081':

        Enter authorization information for "Web API" at reviews.apache.org

        Your review request still exists, but the diff is not attached.

        Creating diff against origin/trunk and uploading patch to JIRA KAFKA-42
        Created a new reviewboard

        It attached the diff, but did not create a RB.

        Show
        Swapnil Ghike added a comment - - edited Saw new issue on RHEL when I tried 'python kafka-patch-review.py -b origin/trunk -j KAFKA-42 -r 14081': Enter authorization information for "Web API" at reviews.apache.org Your review request still exists, but the diff is not attached. Creating diff against origin/trunk and uploading patch to JIRA KAFKA-42 Created a new reviewboard It attached the diff, but did not create a RB.
        Neha Narkhede made changes -
        Attachment KAFKA-1053_2013-09-16_14:40:15.patch [ 12603453 ]
        Hide
        Neha Narkhede added a comment -
        Show
        Neha Narkhede added a comment - Updated reviewboard https://reviews.apache.org/r/14091/
        Hide
        Joel Koshy added a comment -

        Nice - I tried this on KAFKA-1049 (as a test - that patch does not work) and it worked great!

        +1

        I did not get time to dig into the issue I ran into on Linux but the steps worked on my laptop. I can look into that and update the wiki with a work-around if I find one.

        Minor comment: the direct Python API is interesting http://www.reviewboard.org/docs/rbtools/dev/api/overview (I'm in general wary of popen/subprocess); but it is probably more work than its worth to interface with that and post-review likely wraps that anyway and is a well-maintained tool. Also, would prefer to have the tool create a os.tmpfile as opposed to leaving around a patch file but not a big deal.

        Show
        Joel Koshy added a comment - Nice - I tried this on KAFKA-1049 (as a test - that patch does not work) and it worked great! +1 I did not get time to dig into the issue I ran into on Linux but the steps worked on my laptop. I can look into that and update the wiki with a work-around if I find one. Minor comment: the direct Python API is interesting http://www.reviewboard.org/docs/rbtools/dev/api/overview (I'm in general wary of popen/subprocess); but it is probably more work than its worth to interface with that and post-review likely wraps that anyway and is a well-maintained tool. Also, would prefer to have the tool create a os.tmpfile as opposed to leaving around a patch file but not a big deal.
        Neha Narkhede made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Neha Narkhede made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Hide
        Neha Narkhede added a comment - - edited

        Thanks for the reviews.

        Joel -

        Moved the patch to tempdir. Moving to the python API for reviewboard would be great, filed KAFKA-1058 to address that.

        Checked in the tool with the tempdir fix.

        Show
        Neha Narkhede added a comment - - edited Thanks for the reviews. Joel - Moved the patch to tempdir. Moving to the python API for reviewboard would be great, filed KAFKA-1058 to address that. Checked in the tool with the tempdir fix.
        Neha Narkhede made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Neha Narkhede made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Neha Narkhede
            Reporter:
            Neha Narkhede
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development