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...@apache.org> on 2019/02/28 18:53:29 UTC
Re: Review Request 70060: Updated ReviewBot to verify reviews by
checking for updates recursively.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70060/
-----------------------------------------------------------
(Updated Feb. 28, 2019, 6:53 p.m.)
Review request for mesos, Benjamin Bannier and Till Toenshoff.
Changes
-------
Re-implemented the logic to make it easier to read. PTAL.
It's easiest to read the diff between origin and 2. https://reviews.apache.org/r/70060/diff/2/
Summary (updated)
-----------------
Updated ReviewBot to verify reviews by checking for updates recursively.
Bugs: MESOS-4599
https://issues.apache.org/jira/browse/MESOS-4599
Repository: mesos
Description (updated)
-------
If any of the dependent reviews has an updated diff or dependency, it
now triggers the ReviewBot. Previously only updates to the tail
review in the chain triggered the ReviewBot.
Diffs (updated)
-----
support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554
Diff: https://reviews.apache.org/r/70060/diff/2/
Changes: https://reviews.apache.org/r/70060/diff/1-2/
Testing (updated)
-------
? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify
Checking if review: 65835 needs verification
Skipping blocking review 65835
Checking if review: 65836 needs verification
Latest review timestamp: 2018-02-28 14:43:21
Latest diff timestamp: 2018-02-28 13:55:43
Dependent review: https://reviews.apache.org/api/review-requests/65835/
Latest diff timestamp: 2018-02-28 13:55:32
Dependent review: https://reviews.apache.org/api/review-requests/65834/
Latest diff timestamp: 2018-02-28 13:55:22
Checking if review: 65820 needs verification
Skipping blocking review 65820
Checking if review: 65821 needs verification
Skipping blocking review 65821
Checking if review: 65847 needs verification
Latest review timestamp: 2018-03-01 02:54:04
Latest diff timestamp: 2018-02-28 20:22:13
Dependent review: https://reviews.apache.org/api/review-requests/65845/
Latest diff timestamp: 2018-02-28 19:57:14
Dependent review: https://reviews.apache.org/api/review-requests/65844/
Latest diff timestamp: 2018-02-28 19:57:08
Dependent review: https://reviews.apache.org/api/review-requests/65821/
Latest diff timestamp: 2018-02-28 19:57:02
Dependent review: https://reviews.apache.org/api/review-requests/65820/
...
...
Thanks,
Vinod Kone
Re: Review Request 70060: Updated ReviewBot to verify reviews by
checking for updates recursively.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70060/#review213333
-----------------------------------------------------------
Fix it, then Ship it!
support/verify-reviews.py
Lines 51 (patched)
<https://reviews.apache.org/r/70060/#comment299283>
Would could introduce a small function to perform the decoding instead of introducing this global, but very specific constant, e.g.,
def parse_reviewtime(timestamp):
return datetime.strptime(timestamp, "%Y-%m-%dT%H:%M:%SZ")
support/verify-reviews.py
Lines 307 (patched)
<https://reviews.apache.org/r/70060/#comment299286>
Not a strong sentiment, but this function could also be called e.g., `changed_after` or similar since it exclusively deals with timestamps.
support/verify-reviews.py
Lines 308 (patched)
<https://reviews.apache.org/r/70060/#comment299281>
`Returns whether this review request chain needs to be verified.`
support/verify-reviews.py
Lines 310 (patched)
<https://reviews.apache.org/r/70060/#comment299280>
The references to `USER` seem leaky and unnecessary; could we remove them?
Here and below.
support/verify-reviews.py
Line 307 (original), 349 (patched)
<https://reviews.apache.org/r/70060/#comment299282>
`s/True/whether/`
support/verify-reviews.py
Line 330 (original), 359 (patched)
<https://reviews.apache.org/r/70060/#comment299285>
Not yours, but it seems this should page through all open reviews.
support/verify-reviews.py
Line 339 (original), 368 (patched)
<https://reviews.apache.org/r/70060/#comment299284>
Nit: Let's move the comment above the conditional and expand it, e.g.,
# We verify the review if there was no previous review.
- Benjamin Bannier
On Feb. 28, 2019, 7:53 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70060/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2019, 7:53 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
>
>
> Bugs: MESOS-4599
> https://issues.apache.org/jira/browse/MESOS-4599
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If any of the dependent reviews has an updated diff or dependency, it
> now triggers the ReviewBot. Previously only updates to the tail
> review in the chain triggered the ReviewBot.
>
>
> Diffs
> -----
>
> support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554
>
>
> Diff: https://reviews.apache.org/r/70060/diff/2/
>
>
> Testing
> -------
>
> ? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify
> Checking if review: 65835 needs verification
> Skipping blocking review 65835
> Checking if review: 65836 needs verification
> Latest review timestamp: 2018-02-28 14:43:21
> Latest diff timestamp: 2018-02-28 13:55:43
> Dependent review: https://reviews.apache.org/api/review-requests/65835/
> Latest diff timestamp: 2018-02-28 13:55:32
> Dependent review: https://reviews.apache.org/api/review-requests/65834/
> Latest diff timestamp: 2018-02-28 13:55:22
> Checking if review: 65820 needs verification
> Skipping blocking review 65820
> Checking if review: 65821 needs verification
> Skipping blocking review 65821
> Checking if review: 65847 needs verification
> Latest review timestamp: 2018-03-01 02:54:04
> Latest diff timestamp: 2018-02-28 20:22:13
> Dependent review: https://reviews.apache.org/api/review-requests/65845/
> Latest diff timestamp: 2018-02-28 19:57:14
> Dependent review: https://reviews.apache.org/api/review-requests/65844/
> Latest diff timestamp: 2018-02-28 19:57:08
> Dependent review: https://reviews.apache.org/api/review-requests/65821/
> Latest diff timestamp: 2018-02-28 19:57:02
> Dependent review: https://reviews.apache.org/api/review-requests/65820/
> ...
> ...
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 70060: Updated ReviewBot to verify reviews by
checking for updates recursively.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70060/#review213316
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70060]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 28, 2019, 6:53 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70060/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2019, 6:53 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
>
>
> Bugs: MESOS-4599
> https://issues.apache.org/jira/browse/MESOS-4599
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If any of the dependent reviews has an updated diff or dependency, it
> now triggers the ReviewBot. Previously only updates to the tail
> review in the chain triggered the ReviewBot.
>
>
> Diffs
> -----
>
> support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554
>
>
> Diff: https://reviews.apache.org/r/70060/diff/2/
>
>
> Testing
> -------
>
> ? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify
> Checking if review: 65835 needs verification
> Skipping blocking review 65835
> Checking if review: 65836 needs verification
> Latest review timestamp: 2018-02-28 14:43:21
> Latest diff timestamp: 2018-02-28 13:55:43
> Dependent review: https://reviews.apache.org/api/review-requests/65835/
> Latest diff timestamp: 2018-02-28 13:55:32
> Dependent review: https://reviews.apache.org/api/review-requests/65834/
> Latest diff timestamp: 2018-02-28 13:55:22
> Checking if review: 65820 needs verification
> Skipping blocking review 65820
> Checking if review: 65821 needs verification
> Skipping blocking review 65821
> Checking if review: 65847 needs verification
> Latest review timestamp: 2018-03-01 02:54:04
> Latest diff timestamp: 2018-02-28 20:22:13
> Dependent review: https://reviews.apache.org/api/review-requests/65845/
> Latest diff timestamp: 2018-02-28 19:57:14
> Dependent review: https://reviews.apache.org/api/review-requests/65844/
> Latest diff timestamp: 2018-02-28 19:57:08
> Dependent review: https://reviews.apache.org/api/review-requests/65821/
> Latest diff timestamp: 2018-02-28 19:57:02
> Dependent review: https://reviews.apache.org/api/review-requests/65820/
> ...
> ...
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 70060: Updated ReviewBot to verify reviews by
checking for updates recursively.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70060/#review213319
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70060]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 28, 2019, 6:53 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70060/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2019, 6:53 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
>
>
> Bugs: MESOS-4599
> https://issues.apache.org/jira/browse/MESOS-4599
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If any of the dependent reviews has an updated diff or dependency, it
> now triggers the ReviewBot. Previously only updates to the tail
> review in the chain triggered the ReviewBot.
>
>
> Diffs
> -----
>
> support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554
>
>
> Diff: https://reviews.apache.org/r/70060/diff/2/
>
>
> Testing
> -------
>
> ? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify
> Checking if review: 65835 needs verification
> Skipping blocking review 65835
> Checking if review: 65836 needs verification
> Latest review timestamp: 2018-02-28 14:43:21
> Latest diff timestamp: 2018-02-28 13:55:43
> Dependent review: https://reviews.apache.org/api/review-requests/65835/
> Latest diff timestamp: 2018-02-28 13:55:32
> Dependent review: https://reviews.apache.org/api/review-requests/65834/
> Latest diff timestamp: 2018-02-28 13:55:22
> Checking if review: 65820 needs verification
> Skipping blocking review 65820
> Checking if review: 65821 needs verification
> Skipping blocking review 65821
> Checking if review: 65847 needs verification
> Latest review timestamp: 2018-03-01 02:54:04
> Latest diff timestamp: 2018-02-28 20:22:13
> Dependent review: https://reviews.apache.org/api/review-requests/65845/
> Latest diff timestamp: 2018-02-28 19:57:14
> Dependent review: https://reviews.apache.org/api/review-requests/65844/
> Latest diff timestamp: 2018-02-28 19:57:08
> Dependent review: https://reviews.apache.org/api/review-requests/65821/
> Latest diff timestamp: 2018-02-28 19:57:02
> Dependent review: https://reviews.apache.org/api/review-requests/65820/
> ...
> ...
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 70060: Updated ReviewBot to verify reviews by
checking for updates recursively.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70060/
-----------------------------------------------------------
(Updated March 1, 2019, 10:11 p.m.)
Review request for mesos, Benjamin Bannier and Till Toenshoff.
Changes
-------
Addressed bannier's comments. NNFR.
Bugs: MESOS-4599
https://issues.apache.org/jira/browse/MESOS-4599
Repository: mesos
Description
-------
If any of the dependent reviews has an updated diff or dependency, it
now triggers the ReviewBot. Previously only updates to the tail
review in the chain triggered the ReviewBot.
Diffs (updated)
-----
support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554
Diff: https://reviews.apache.org/r/70060/diff/3/
Changes: https://reviews.apache.org/r/70060/diff/2-3/
Testing
-------
? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify
Checking if review: 65835 needs verification
Skipping blocking review 65835
Checking if review: 65836 needs verification
Latest review timestamp: 2018-02-28 14:43:21
Latest diff timestamp: 2018-02-28 13:55:43
Dependent review: https://reviews.apache.org/api/review-requests/65835/
Latest diff timestamp: 2018-02-28 13:55:32
Dependent review: https://reviews.apache.org/api/review-requests/65834/
Latest diff timestamp: 2018-02-28 13:55:22
Checking if review: 65820 needs verification
Skipping blocking review 65820
Checking if review: 65821 needs verification
Skipping blocking review 65821
Checking if review: 65847 needs verification
Latest review timestamp: 2018-03-01 02:54:04
Latest diff timestamp: 2018-02-28 20:22:13
Dependent review: https://reviews.apache.org/api/review-requests/65845/
Latest diff timestamp: 2018-02-28 19:57:14
Dependent review: https://reviews.apache.org/api/review-requests/65844/
Latest diff timestamp: 2018-02-28 19:57:08
Dependent review: https://reviews.apache.org/api/review-requests/65821/
Latest diff timestamp: 2018-02-28 19:57:02
Dependent review: https://reviews.apache.org/api/review-requests/65820/
...
...
Thanks,
Vinod Kone