You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2016/02/19 01:38:42 UTC
Review Request 43747: Fixed ReviewBot to catch circular dependencies
in review requests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43747/
-----------------------------------------------------------
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
Re: Review Request 43747: Fixed ReviewBot to catch circular
dependencies in review requests.
Posted by Kevin Klues <kl...@gmail.com>.
> On Feb. 19, 2016, 1:16 a.m., Kevin Klues wrote:
> > 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.
Diff here of applied changes from the review:
https://github.com/klueska-mesosphere/mesos/commit/ca1ca4a53bbd311d2a73a7ccdcca21f03147bbc5
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43747/#review119759
-----------------------------------------------------------
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
>
>
Re: Review Request 43747: Fixed ReviewBot to catch circular
dependencies in review requests.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 43747: Fixed ReviewBot to catch circular
dependencies in review requests.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43747/#review119814
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43747]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>