Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-709

Add .reviewboardrc and use rbt instead of post-review

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.0
    • Component/s: conf and scripts
    • Labels:
      None

      Description

      The request-patch-review.py script uses the post-review tool to post a patch to Review Board, which is deprecated in favor of rbt. The instructions on setting up configuration for submitting posts is also slightly complex and can be made simpler by adding a .reviewboardrc to source control.

      1. TAJO-709_20140402_10:39:48.patch
        16 kB
        David Chen
      2. TAJO-709.patch
        17 kB
        David Chen

        Activity

        Hide
        davidzchen David Chen added a comment -

        Created a review request against branch master in reviewboard
        https://reviews.apache.org/r/19863/

        Show
        davidzchen David Chen added a comment - Created a review request against branch master in reviewboard https://reviews.apache.org/r/19863/
        Hide
        davidzchen David Chen added a comment -

        I think this is ready for a preliminary review. The main changes are:

        • Some style cleanup for readability.
        • Break up the large main() into multiple smaller functions to make the logic of the script more clear.
        • Add .reviewboardrc and use rbt instead of post-review, which is deprecated
        • Read tracking branch from .reviewboardrc and make -b --branch optional and used only if the user wishes to specify a different tracking branch than the default.

        The file now appears longer but not because there is logic but rather because most lines are now under 80 characters in length.

        Posting a review is now as easy as: ./request-patch-review.py -j TAJO-709 since the default branch (origin/master) is read from .reviewboardrc.

        Possible additional considerations:

        • I am getting a Internal Server Error 500 when running the script with -pa when the script calls jira.transition_issue. I do not think this is caused by my changes because I got the same error when running the original script without my changes.
        • I don't completely like the way I am passing so many parameters around. This can be made cleaner using a class but that might be overengineering.
        Show
        davidzchen David Chen added a comment - I think this is ready for a preliminary review. The main changes are: Some style cleanup for readability. Break up the large main() into multiple smaller functions to make the logic of the script more clear. Add .reviewboardrc and use rbt instead of post-review, which is deprecated Read tracking branch from .reviewboardrc and make -b --branch optional and used only if the user wishes to specify a different tracking branch than the default. The file now appears longer but not because there is logic but rather because most lines are now under 80 characters in length. Posting a review is now as easy as: ./request-patch-review.py -j TAJO-709 since the default branch (origin/master) is read from .reviewboardrc. Possible additional considerations: I am getting a Internal Server Error 500 when running the script with -pa when the script calls jira.transition_issue . I do not think this is caused by my changes because I got the same error when running the original script without my changes. I don't completely like the way I am passing so many parameters around. This can be made cleaner using a class but that might be overengineering.
        Hide
        tajoqa Tajo QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12637980/TAJO-709.patch
        against master revision bd418a5.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

        +1 checkstyle. The patch generated 0 code style errors.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/292//testReport/
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/292//console

        This message is automatically generated.

        Show
        tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12637980/TAJO-709.patch against master revision bd418a5. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/292//testReport/ Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/292//console This message is automatically generated.
        Hide
        hyunsik Hyunsik Choi added a comment -

        +1

        The patch looks pretty nice to me. The readability is significantly improved, and the script becomes more easy to use. Thank you for your work!

        Actually, I haven't succeeded -pa command of this script even though the command seems to work with the parameter in {{jiracli}. I added this command to the script, but I don't think so anyone need this feature. So, I think that we don't need to mind the feature, and I'm also Ok even if it is removed from the script.

        If you think that the patch is ready to commit, I'll commit it.

        Best regards,
        Hyunsik Choi

        Show
        hyunsik Hyunsik Choi added a comment - +1 The patch looks pretty nice to me. The readability is significantly improved, and the script becomes more easy to use. Thank you for your work! Actually, I haven't succeeded -pa command of this script even though the command seems to work with the parameter in {{jiracli}. I added this command to the script, but I don't think so anyone need this feature. So, I think that we don't need to mind the feature, and I'm also Ok even if it is removed from the script. If you think that the patch is ready to commit, I'll commit it. Best regards, Hyunsik Choi
        Hide
        davidzchen David Chen added a comment -

        Updated the review request against branch master in reviewboard
        https://reviews.apache.org/r/19863/

        Show
        davidzchen David Chen added a comment - Updated the review request against branch master in reviewboard https://reviews.apache.org/r/19863/
        Hide
        davidzchen David Chen added a comment -

        Thanks, Hyunsik. I have removed the -pa option for now. If there is enough demand for it in the future, we can look at restoring that functionality and try to get it to work.

        I have rebased on master and submitted an updated patch. I think this is ready to be committed.

        Thanks,
        David

        Show
        davidzchen David Chen added a comment - Thanks, Hyunsik. I have removed the -pa option for now. If there is enough demand for it in the future, we can look at restoring that functionality and try to get it to work. I have rebased on master and submitted an updated patch. I think this is ready to be committed. Thanks, David
        Hide
        tajoqa Tajo QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12638297/TAJO-709_20140402_10%3A39%3A48.patch
        against master revision dc40d84.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

        +1 checkstyle. The patch generated 0 code style errors.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/303//testReport/
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/303//console

        This message is automatically generated.

        Show
        tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12638297/TAJO-709_20140402_10%3A39%3A48.patch against master revision dc40d84. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/303//testReport/ Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/303//console This message is automatically generated.
        Hide
        hyunsik Hyunsik Choi added a comment -

        +1 for the latest patch.

        Show
        hyunsik Hyunsik Choi added a comment - +1 for the latest patch.
        Hide
        hyunsik Hyunsik Choi added a comment -

        committed it to master branch. Thank you David!

        Show
        hyunsik Hyunsik Choi added a comment - committed it to master branch. Thank you David!
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #154 (See https://builds.apache.org/job/Tajo-master-build/154/)
        TAJO-709: Add .reviewboardrc and use rbt instead of post-review. (David Chen via hyunsik) (hyunsik: rev e036ea36dae13c0350f0d84e673f71b402986a48)

        • CHANGES.txt
        • request-patch-review.py
        • .reviewboardrc
        • .gitignore
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #154 (See https://builds.apache.org/job/Tajo-master-build/154/ ) TAJO-709 : Add .reviewboardrc and use rbt instead of post-review. (David Chen via hyunsik) (hyunsik: rev e036ea36dae13c0350f0d84e673f71b402986a48) CHANGES.txt request-patch-review.py .reviewboardrc .gitignore

          People

          • Assignee:
            davidzchen David Chen
            Reporter:
            davidzchen David Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development