----------------------------------------------------------- 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 > >
