Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============6368593962967835259=="
MIME-Version: 1.0
Subject: Re: Review Request 38705: Added support for applying a review chain
 (apply-reviews.py).
From: "Marco Massenzio" <marco@mesosphere.io>
To: "Joris Van Remoortere" <joris.van.remoortere@gmail.com>,
 "Benjamin Hindman" <benh@berkeley.edu>, "Joseph Wu" <joseph@mesosphere.io>,
 "Vinod Kone" <vinodkone@gmail.com>
Cc: "Artem Harutyunyan" <artem@mesosphere.io>,
 "mesos" <reviews@mesos.apache.org>, "Marco Massenzio" <marco@mesosphere.io>
Date: Fri, 16 Oct 2015 16:53:33 -0000
Message-ID: <20151016165333.28635.51056@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Marco Massenzio" <noreply@reviews.apache.org>
References: <20151014125523.28636.94507@reviews.apache.org>
In-Reply-To: <20151014125523.28636.94507@reviews.apache.org>
Reply-To: "Marco Massenzio" <marco@mesosphere.io>

--===============6368593962967835259==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 7
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line7>
> >
> >     uhm... could we use `requests` instead?
> >     much more modern API and widespread use.
> 
> Artem Harutyunyan wrote:
>     `requests` looks great, but it looks like it requires to be installed separately. I would really like to restrict myself to only the modules available with stock python distribution. Ditto for `sh`.

right, agree.
however, as I remarked in the other review, it would not be a major hurdle to get folks to pip install those two.

we demand they install a ton-worth of C++ and other assorted libraries, so asking to create a virtualenv and/or run `pip install sh requests` does not appear to me to be huge demand :)

(also, appy to write a 'checker' that can be run prior to these scripts and does the needful)


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 19
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line19>
> >
> >     you should check for a non 4xx/5xx return code (or just check for a 2xx if you know for sure what the API returns).
> >     
> >     as you expect JSON, you should probably specify that in the `Accept-content` header too?
> 
> Artem Harutyunyan wrote:
>     urllib2 throws an exception if an error occurs, which forces the program to terminate. Termination is the right thing to do here, because there is no recovery path, so I would only want to catch the exception to print a pretty message, but the stack trace should be informative enough for the developer who is running this script. 
>     As for the `content-type`, ReviewBoard is setting it to something unorthodox (`Content-Type: application/vnd.reviewboard.org.review-request+json`), do you think we should verify against that specific value?

uh? really? fascinating...
what I was suggesting, though, was in the **Request** (asking the server to send you back JSON) in which case you either do get JSON (and don't need to check the `Content-Type`) or get a 415.

If you don't tell the server, then, yes, you have to check what the guy threw at you, as it's not in principle possible to know in advance what the default mimetype is.


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 45
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line45>
> >
> >     is this right?
> >     you return after the first iteration?
> >     
> >     if so, why not just getting the first item in `depends_on`?
> 
> Artem Harutyunyan wrote:
>     I think this is right. That statement is after the recursive call, so actually when in gets there for the last time the lisst contains the entire chain (whereas `depends_on` only contains immediate anchestor(s)).

weird.
not sure I completely like that - but I'll take your word for it that it works :)

do you mind terribly to add a comment, so we don't risk someone "fix it" by unindenting it sometime in the future?


> On Oct. 14, 2015, 12:55 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 56
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line56>
> >
> >     if you do this, this script will be probably useful to folks who use Python 3 too :)
> >     
> >     ```
> >     from __future__ import print_function
> >     
> >     ...
> >     
> >     print('foo bar')
> >     ```
> >     Also (but that's just something a bunch of us insisted upon) I prefer the use of `string.format()` to `%`:
> >     ```
> >     print("Applying review {}: {}".format(review_id, summary))
> >     ```
> >     (same also below to build `cmd`)
> 
> Artem Harutyunyan wrote:
>     I am not sure whether we should use python 3. Other python scripts in Mesos repo seem to be written for 2.x versions, so I'd like to stay consistent.

This works with Python 2.7 too (that's the point of the `import __future__`)


- Marco


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


On Oct. 16, 2015, 6:50 a.m., Artem Harutyunyan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2015, 6:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3468
>     https://issues.apache.org/jira/browse/MESOS-3468
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   support/apply-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38705/diff/
> 
> 
> Testing
> -------
> 
> Tested the script with python 2.7.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>


--===============6368593962967835259==--