mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 70000: Fixed verify-reviews.py to not abort review verification prematurely.
Date Tue, 19 Feb 2019 16:53:52 GMT


> On Feb. 19, 2019, 5:47 p.m., Vinod Kone wrote:
> > support/verify-reviews.py
> > Line 182 (original), 188 (patched)
> > <https://reviews.apache.org/r/70000/diff/3/?file=2125836#file2125836line193>
> >
> >     hmm. i don't think this is the right place to catch this exception. what if
we don't get into this for loop at all if it's a single review chain?
> >     
> >     i think it should be in #197.

Good point. It seems it _also_ needs to be in l.197, but we still need to handle errors on
l.188 as a patch from the dependencies might not apply (that's what we see currently). In
that case we unfortunately post the message on the wrong patch until MESOS-9583 is fixed.


- Benjamin


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


On Feb. 19, 2019, 5:53 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70000/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2019, 5:53 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-9582
>     https://issues.apache.org/jira/browse/MESOS-9582
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously `support/verify-reviews.py` would abort prematurely
> whenever a review a patch could not be applied. This was due to
> the `shell` function used to call `support/apply-reviews.py` invoking
> `exit(1)` on the first error which stopped iteration over all
> outstanding reviews.
> 
> In this patch that explicit call to `exit` is removed, and instead we
> let a possible `subprocess.CalledProcessError` propagate up for it to
> be handled at a higher level. Currently this will post a note on the
> review in question to (1) notify the submitter, and (2) prevent the
> review from being checked again.
> 
> With the changes here the script will not stop verify reviews in such
> cases.
> 
> 
> Diffs
> -----
> 
>   support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 
> 
> 
> Diff: https://reviews.apache.org/r/70000/diff/4/
> 
> 
> Testing
> -------
> 
> Ran the script (with a dummy user and password) on today's reviewboard state. The script
attempted to post a review on the last patch in the chain instead of aborting (see the `TODO`
in the code on why we weren't able to diagnose the issue in the faulty patch with the current
implementation).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message