-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43747/#review119759
-----------------------------------------------------------


Fix it, then Ship it!




If we wren't using reviewboard (which I find overly cumbersome sometimes), I 
would probably have split this into two commits.  One for the logic change 
(throwing an error), and one for the variable name change (applied->reviews). 
It's probably a little much for something this small though.


support/verify_reviews.py (lines 83 - 94)
<https://reviews.apache.org/r/43747/#comment181115>

    I would move the error check for the reviewers first, so that the circular 
dependency check can be grouped with adding the current request to the reviews 
list. I would even put an else to make it more clear what's going on (even 
though it's technically not necessary). i.e.:
    
    ```
        # If there is a circular dependency throw an error.`
        if review_request["id"] in reviews:
            raise ReviewError("Circular dependency detected for review %s."
                              "Please fix the 'depends_on' field."
                              % review_request["id"])
        else:
            reviews.append(review_request["id"])
    ```



support/verify_reviews.py (lines 89 - 91)
<https://reviews.apache.org/r/43747/#comment181113>

    I would move this as the first error case. I would also add a comment for 
consistency.


- Kevin Klues


On Feb. 19, 2016, 12:38 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43747/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2016, 12:38 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Kevin Klues.
> 
> 
> Bugs: MESOS-4713
>     https://issues.apache.org/jira/browse/MESOS-4713
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixed ReviewBot to catch circular dependencies in review requests.
> 
> 
> Diffs
> -----
> 
>   support/verify_reviews.py f0b9996b04e29e5d70ff57784c46d00e5ec4e45c 
> 
> Diff: https://reviews.apache.org/r/43747/diff/
> 
> 
> Testing
> -------
> 
> Tested locally with a circular dep.
> 
> ?  mesos git:(vinod/reviewbot_recursive) BUILD_URL=foo 
> ./support/verify_reviews.py kone vinod 1 
> "?to-groups=mesos&status=pending&start=2"
> git rev-parse HEAD
> Checking if review: 43418 needs verification
> Skipping blocking review 43418
> Checking if review: 43691 needs verification
> Skipping blocking review 43691
> Checking if review: 43689 needs verification
> Skipping blocking review 43689
> Checking if review: 43697 needs verification
> Skipping blocking review 43697
> Checking if review: 43698 needs verification
> Skipping blocking review 43698
> Checking if review: 43699 needs verification
> Skipping blocking review 43699
> Checking if review: 43692 needs verification
> Skipping blocking review 43692
> Checking if review: 43693 needs verification
> Skipping blocking review 43693
> Checking if review: 43694 needs verification
> Skipping blocking review 43694
> Checking if review: 43695 needs verification
> Skipping blocking review 43695
> Checking if review: 43271 needs verification
> Skipping blocking review 43271
> Checking if review: 43272 needs verification
> Latest diff timestamp: 2016-02-18 19:19:48
> Verifying review 43272
> Dependent review: https://reviews.apache.org/api/review-requests/43271/ 
> Dependent review: https://reviews.apache.org/api/review-requests/43261/ 
> Dependent review: https://reviews.apache.org/api/review-requests/43271/ 
> Posting review: Bad review!
> 
> Reviews applied: [43272, 43271, 43261]
> 
> Error:
> Circular dependency detected for review 43271.Please fix the 'depends_on' 
> field.
> Error handling URL 
> https://reviews.apache.org/api/review-requests/43272/reviews/: UNAUTHORIZED 
> ({"stat": "fail", "err": {"msg": "The username or password was not correct", 
> "code": 104}})
> git clean -fd
> git reset --hard fb779576521abc35d99f2b8834684f3a8f020895
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Reply via email to