You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Artem Harutyunyan <ar...@mesosphere.io> on 2015/09/30 08:09:14 UTC
Review Request 38883: Removed calls to apply-review.sh script. Added
support for amending commit messages.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
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/38883/diff/
Testing
-------
tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review101092
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705, 38883]
All tests passed.
- Mesos ReviewBot
On Sept. 30, 2015, 6:18 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 6:18 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102000
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705, 38883]
All tests passed.
- Mesos ReviewBot
On Oct. 8, 2015, 6:28 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 6:28 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Oct. 16, 2015, 4:15 p.m., Marco Massenzio wrote:
> > Ship It!
Thanks for addressing comments!
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102921
-----------------------------------------------------------
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 3:29 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Oct. 16, 2015, 4:15 p.m., Marco Massenzio wrote:
> > Ship It!
>
> Marco Massenzio wrote:
> Thanks for addressing comments!
hey, minor nit: I've just noticed you insert whitespaces around named args:
```
"....".format(url = USER_URL, user = username)
```
PEP8[0] requires that there are no spaces around the `=`:
```
format(url=USER_URL, user=username)
```
also in the default values around method params:
```
def do_it(name="foof", bar=None):
```
if you can fix before committing that'd be grand (but no big deal).
Thanks!
[0] https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements
("""Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.""")
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102921
-----------------------------------------------------------
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 3:29 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102921
-----------------------------------------------------------
Ship it!
Ship It!
- Marco Massenzio
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 3:29 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102925
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705, 38883]
All tests passed.
- Mesos ReviewBot
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 3:29 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104098
-----------------------------------------------------------
support/apply-reviews.py (line 18)
<https://reviews.apache.org/r/38883/#comment162323>
With the format string you're using, this note isn't _as_ necessary. (You could remove it.) Ditto with the other similar comments.
support/apply-reviews.py (line 19)
<https://reviews.apache.org/r/38883/#comment162322>
Note: This would also resolve the issue I had with this line in the previous review.
support/apply-reviews.py (line 41)
<https://reviews.apache.org/r/38883/#comment162326>
Seems like this could go into the previous review. Same for the next 2 commenting changes/additions.
- Joseph Wu
On Oct. 22, 2015, 11:18 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2015, 11:18 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review103778
-----------------------------------------------------------
Ship it!
Ship It!
- Marco Massenzio
On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 6:18 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104828
-----------------------------------------------------------
support/apply-reviews.py (line 63)
<https://reviews.apache.org/r/38883/#comment163080>
kill the period at the end of the log statement.
here and everywhere else.
support/apply-reviews.py (line 75)
<https://reviews.apache.org/r/38883/#comment163081>
ditto.
kill the period at the end of the log statement.
support/apply-reviews.py (line 120)
<https://reviews.apache.org/r/38883/#comment163083>
i would put the "."on the next line.
support/apply-reviews.py (line 133)
<https://reviews.apache.org/r/38883/#comment163084>
ditto. "." on the nextline.
support/apply-reviews.py (line 144)
<https://reviews.apache.org/r/38883/#comment163085>
dot on the next line.
support/apply-reviews.py (line 155)
<https://reviews.apache.org/r/38883/#comment163086>
".format" should be aligned with '{summary..' ?
- Vinod Kone
On Oct. 30, 2015, 8:54 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2015, 8:54 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Nov. 11, 2015, 5:56 a.m., Ben Mahler wrote:
> > support/apply-reviews.py, lines 158-164
> > <https://reviews.apache.org/r/38883/diff/16/?file=1119827#file1119827line158>
> >
> > The url added to the message here doesn't match the existing format used by apply-review.sh:
> >
> > ```
> > Review: https://reviews.apache.org/api/review-requests/38876/
> > ```
> >
> > vs the expected:
> >
> > ```
> > Review: https://reviews.apache.org/r/38876
> > ```
Fixed in https://reviews.apache.org/r/40225/.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review106053
-----------------------------------------------------------
On Nov. 9, 2015, 6:18 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2015, 6:18 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review106053
-----------------------------------------------------------
support/apply-reviews.py (lines 148 - 154)
<https://reviews.apache.org/r/38883/#comment164778>
The url added to the message here doesn't match the existing format used by apply-review.sh:
```
Review: https://reviews.apache.org/api/review-requests/38876/
```
vs the expected:
```
Review: https://reviews.apache.org/r/38876
```
- Ben Mahler
On Nov. 9, 2015, 2:18 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2015, 2:18 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 6:18 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Quoting shell variables; Cleanup.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 2:57 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
More cleanups.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 12:29 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Cleanups.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 3 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Got rid of urljoin.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 2:30 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104829
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Oct. 30, 2015, 8:54 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2015, 8:54 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 30, 2015, 1:54 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Made `options` global, cleaned up.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 28, 2015, 3:01 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Addressed some comments.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 26, 2015, 4:22 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 123
> > <https://reviews.apache.org/r/38883/diff/9/?file=1103869#file1103869line123>
> >
> > do you still need this if it's already in the atexit handler?
I do, because atexit is called only once (for the last review) whereas I need to clean up the intermediate patch files after applying reviews in the chain.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104106
-----------------------------------------------------------
On Oct. 28, 2015, 3:01 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2015, 3:01 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 26, 2015, 4:22 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 79
> > <https://reviews.apache.org/r/38883/diff/9/?file=1103869#file1103869line79>
> >
> > s/options/dry_run/
> >
> > you can generalize it to 'options' later if there is a need for it.
I made `options` global. It makes things easier.
> On Oct. 26, 2015, 4:22 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 106
> > <https://reviews.apache.org/r/38883/diff/9/?file=1103869#file1103869line106>
> >
> > ditto. just take 'dry_run' as an argument.
See previous comment.
> On Oct. 26, 2015, 4:22 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, lines 126-142
> > <https://reviews.apache.org/r/38883/diff/9/?file=1103869#file1103869line126>
> >
> > i'm not sure these need to be functions. you can just inline them.
I could, but then it would make calling functions less readable. If you feel strongly about this one I can go ahead and change, for now I'll just drop.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104106
-----------------------------------------------------------
On Oct. 30, 2015, 1:54 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2015, 1:54 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review104106
-----------------------------------------------------------
support/apply-reviews.py (line 41)
<https://reviews.apache.org/r/38883/#comment162331>
capitalization of "Review ID" is different from above. please stick with one.
support/apply-reviews.py (lines 56 - 57)
<https://reviews.apache.org/r/38883/#comment162332>
bad rebase?
support/apply-reviews.py (line 78)
<https://reviews.apache.org/r/38883/#comment162333>
s/options/dry_run/
you can generalize it to 'options' later if there is a need for it.
support/apply-reviews.py (line 96)
<https://reviews.apache.org/r/38883/#comment162334>
ditto. just take 'dry_run' as an argument.
support/apply-reviews.py (line 113)
<https://reviews.apache.org/r/38883/#comment162339>
do you still need this if it's already in the atexit handler?
support/apply-reviews.py (lines 116 - 132)
<https://reviews.apache.org/r/38883/#comment162340>
i'm not sure these need to be functions. you can just inline them.
support/apply-reviews.py (line 134)
<https://reviews.apache.org/r/38883/#comment162341>
s/populate_//
support/apply-reviews.py (line 157)
<https://reviews.apache.org/r/38883/#comment162338>
seems weird that a function called amend() checks to see if it can amend or not. i would've imagined the caller to do that.
support/apply-reviews.py (line 173)
<https://reviews.apache.org/r/38883/#comment162337>
typically "-n" is used as a short argument for dry run. but i see why couldn't use it :(
- Vinod Kone
On Oct. 23, 2015, 6:18 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 6:18 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 22, 2015, 11:18 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Rebased.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 12:42 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Rebased.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 18, 2015, 3:31 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
space/tab cleanup.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 17, 2015, 7:39 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Cleanup.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 16, 2015, 8:29 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Removed extra whitespace.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102904
-----------------------------------------------------------
Bad patch!
Reviews applied: [38705, 38883]
Failed command: ./support/apply-review.sh -n -r 38883
Error:
2015-10-16 10:50:05 URL:https://reviews.apache.org/r/38883/diff/raw/ [6369/6369] -> "38883.patch" [1]
38883.patch:160: trailing whitespace.
warning: 1 line adds whitespace errors.
Successfully applied: Removed calls to apply-review.sh script. Added support for amending commit messages.
See summary.
Review: https://reviews.apache.org/r/38883
support/apply-reviews.py:151: trailing whitespace.
+
Failed to commit patch
- Mesos ReviewBot
On Oct. 16, 2015, 7:03 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 7:03 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 16, 2015, 12:03 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 40
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line40>
> >
> > (setting aside for a second that we should use `requests.get()` instead :)
> >
> > can you please that you get a 200 OK code and that there is actually JSON content?
> >
> > with `requests` this is as easy as:
> > ```
> > import requests
> >
> > ...
> > try:
> > headers = {'AcceptContent': "application/json"}
> > r = requests.get(url, timeout=50, headers=headers)
> > if 200 <= r.status_code < 300 and r.headers.get('content-type') == 'application/json':
> > return r.json()
> > else:
> > # do something with error
> > pass
> > except requestsTimeoutError:
> > # log the error and probably give up, the server is down
> > pass
> > ```
>
> Artem Harutyunyan wrote:
> In both cases (if it's not a `200` code, or if it's not a JSON) I want to terminate, and that's exactly what the script will do for me. Since the intended audience are developers I am willingly frugal when it comes to writing code, and tend to resort to stack traces wherever I can. What do you think?
I don't like relying on stack traces (and Python's aren't particularly useful) - even as a developer, the message I derive is "the code is buggy" *not* something went wrong with the service.
Providing a LOG(ERROR) with an explanation of whent went wrong would be a much better experience (and, in this specific case, I bet the stacktrace would be something along the lines of "Unknown JSON content" or something equally misleading).
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 91
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line91>
> >
> > ```
> > from __future__ import print_function:
> >
> > ...
> >
> > print(output)
> > ```
> > (this way it'll all work also for those us using a modern Python interpreter :) )
> >
> > thanks!
>
> Artem Harutyunyan wrote:
> Other scripts in the repo seem to be developed for Python 2.x, and I'd like to stay consistent.
that being exactly my point: what I suggested is consistent with 2.7, but can be used with 3.4 without any change.
have your cake and eat it, too :D
(and it's only good to be consistent with something that's good ;-) )
> On Oct. 14, 2015, 4:45 p.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 102
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line102>
> >
> > consider using the `sh` module:
> > ```
> > import sh
> >
> > cmd = "{rev_id}.patch".format(rev_id=review_id)
> > sh.rm("-f", cmd)
> >
> > ```
>
> Artem Harutyunyan wrote:
> `sh` does not seem to be a stock module.
good point!
Although really easy to fix:
```
try:
import sh
except ImportError:
print("This script requires the use of the `sh` module; please run: `sudo pip install sh`")
exit(1)
```
(especially, as you remarked above, that users are developers, so shouldn't be afraid of doing this)
If we want to be sophisticated, we could even have a `requirements.txt` that we'd run during the ./bootstrap script or document developers to run once.
`pip install --upgrade -r requirements.txt`
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
-----------------------------------------------------------
On Oct. 16, 2015, 3:29 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 3:29 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > Again lots of nit-picking, feel free to ignore what you disagree with strongly :)
> >
> > Also, instead of using your homemade `shell()` method and `Subprocess`, given the usage pattern I've noticed, you may want to consider the `sh` module:
> > http://amoffat.github.io/sh/
> >
> > once you start using it, it gets pretty awesome!
`sh` is indeed awesome, however it's not available by default, and I'd like to stick to stock modules only.
> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 40
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line40>
> >
> > (setting aside for a second that we should use `requests.get()` instead :)
> >
> > can you please that you get a 200 OK code and that there is actually JSON content?
> >
> > with `requests` this is as easy as:
> > ```
> > import requests
> >
> > ...
> > try:
> > headers = {'AcceptContent': "application/json"}
> > r = requests.get(url, timeout=50, headers=headers)
> > if 200 <= r.status_code < 300 and r.headers.get('content-type') == 'application/json':
> > return r.json()
> > else:
> > # do something with error
> > pass
> > except requestsTimeoutError:
> > # log the error and probably give up, the server is down
> > pass
> > ```
In both cases (if it's not a `200` code, or if it's not a JSON) I want to terminate, and that's exactly what the script will do for me. Since the intended audience are developers I am willingly frugal when it comes to writing code, and tend to resort to stack traces wherever I can. What do you think?
> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 91
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line91>
> >
> > ```
> > from __future__ import print_function:
> >
> > ...
> >
> > print(output)
> > ```
> > (this way it'll all work also for those us using a modern Python interpreter :) )
> >
> > thanks!
Other scripts in the repo seem to be developed for Python 2.x, and I'd like to stay consistent.
> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 102
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line102>
> >
> > consider using the `sh` module:
> > ```
> > import sh
> >
> > cmd = "{rev_id}.patch".format(rev_id=review_id)
> > sh.rm("-f", cmd)
> >
> > ```
`sh` does not seem to be a stock module.
> On Oct. 14, 2015, 9:45 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 106
> > <https://reviews.apache.org/r/38883/diff/3/?file=1093293#file1093293line106>
> >
> > can you please explains what is `options` supposed to be / contain?
> > I think you expect a `dict` here, it would be great to have something like...
> >
> > ```
> > """ blah blah
> >
> > @param options: these are the fuz bits that god the baz, in a map that can contain the following keys: [`dry_run`, `verbose`, `bitz`] and if not specified assumes `dry_run` to be False.
> > @type options: dict
> > """
> > ```
Added a comment in main function.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
-----------------------------------------------------------
On Oct. 16, 2015, 12:03 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 12:03 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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102624
-----------------------------------------------------------
Again lots of nit-picking, feel free to ignore what you disagree with strongly :)
Also, instead of using your homemade `shell()` method and `Subprocess`, given the usage pattern I've noticed, you may want to consider the `sh` module:
http://amoffat.github.io/sh/
once you start using it, it gets pretty awesome!
support/apply-reviews.py (line 29)
<https://reviews.apache.org/r/38883/#comment160357>
always best to prefer `str.join()` to join strings; in this case, however, as you need to terminate with the `/` it's best to use `format()`:
```
return "{url}/{user}/".format(url=USER_URL, user=user)
```
of course:
```
'/'.join([USER_URL, user, ''])
```
would have also been acceptable.
support/apply-reviews.py (line 34)
<https://reviews.apache.org/r/38883/#comment160358>
same here, please
support/apply-reviews.py (line 40)
<https://reviews.apache.org/r/38883/#comment160361>
(setting aside for a second that we should use `requests.get()` instead :)
can you please that you get a 200 OK code and that there is actually JSON content?
with `requests` this is as easy as:
```
import requests
...
try:
headers = {'AcceptContent': "application/json"}
r = requests.get(url, timeout=50, headers=headers)
if 200 <= r.status_code < 300 and r.headers.get('content-type') == 'application/json':
return r.json()
else:
# do something with error
pass
except requestsTimeoutError:
# log the error and probably give up, the server is down
pass
```
support/apply-reviews.py (line 59)
<https://reviews.apache.org/r/38883/#comment160366>
this will throw a KeyError if either of the keys are missing (and the user will be none the wiser):
```
if 'review_request' in json_obj and not json_obj['review_request].get('depends_on'):
... do something
```
(also see my comments in the other review about the lines above this)
support/apply-reviews.py (line 78)
<https://reviews.apache.org/r/38883/#comment160367>
```
from __future__ import print_function:
...
print(output)
```
(this way it'll all work also for those us using a modern Python interpreter :) )
thanks!
support/apply-reviews.py (line 86)
<https://reviews.apache.org/r/38883/#comment160390>
please don't do this - prefer the use of `**kwargs` or, in this case, it's much easier to do something like:
```
def remove_patch(review_id, options=None):
options = options or {'dry_run': False}
...
```
support/apply-reviews.py (line 89)
<https://reviews.apache.org/r/38883/#comment160368>
consider using the `sh` module:
```
import sh
cmd = "{rev_id}.patch".format(rev_id=review_id)
sh.rm("-f", cmd)
```
support/apply-reviews.py (line 93)
<https://reviews.apache.org/r/38883/#comment160392>
can you please explains what is `options` supposed to be / contain?
I think you expect a `dict` here, it would be great to have something like...
```
""" blah blah
@param options: these are the fuz bits that god the baz, in a map that can contain the following keys: [`dry_run`, `verbose`, `bitz`] and if not specified assumes `dry_run` to be False.
@type options: dict
"""
```
support/apply-reviews.py (line 107)
<https://reviews.apache.org/r/38883/#comment160394>
prefer the use of `str.format()` instead
(here and everywhere else)
support/apply-reviews.py (lines 126 - 130)
<https://reviews.apache.org/r/38883/#comment160399>
I really really don't like this 'leading underscore' and same-naming: makes reading the code really difficult and, thanks to Python happily ignoring different types, the likelihood of bugs super-high.
also, seems to me that you never use `_review` anywhere else, so that's what I'd do:
```
data = url_to_json(review_url(review_id))
if data:
review = data.get('review_request')
else:
# something bad happened bail
```
same goes for `_user` too.
support/apply-reviews.py (lines 133 - 139)
<https://reviews.apache.org/r/38883/#comment160400>
more pythonic:
```
review_data = {
'summary': review['summary'],
'description': review['description'],
'url': review_url(review_id),
'author': user['fullname'],
'email': user['email']
'message': '\n\n'.join([review['summary'],
review['description'],
'Review : {}'.format(review_data['url'])])
}
```
- Marco Massenzio
On Oct. 8, 2015, 6:28 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 6:28 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review102039
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On 十月 8, 2015, 6:28 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated 十月 8, 2015, 6:28 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Oct. 8, 2015, 11:28 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Rebased; Added long option names.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review101146
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705, 38883]
All tests passed.
- Mesos ReviewBot
On Sept. 30, 2015, 6:21 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 6:21 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Sept. 30, 2015, 11:21 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-3468
https://issues.apache.org/jira/browse/MESOS-3468
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
support/apply-reviews.py PRE-CREATION
Diff: https://reviews.apache.org/r/38883/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/#review101131
-----------------------------------------------------------
Preliminary review.
support/apply-reviews.py (line 27)
<https://reviews.apache.org/r/38883/#comment158469>
Propagated typo: Retutns
support/apply-reviews.py (line 32)
<https://reviews.apache.org/r/38883/#comment158470>
Propagated typo: Retutns
support/apply-reviews.py (line 105)
<https://reviews.apache.org/r/38883/#comment158473>
To stay consistent: s/reviewboard/Review Board/
support/apply-reviews.py (line 132)
<https://reviews.apache.org/r/38883/#comment158475>
Spaces around the `=`.
- Joseph Wu
On Sept. 29, 2015, 11:18 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38883/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 11:18 p.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/38883/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38883: Removed calls to apply-review.sh script.
Added support for amending commit messages.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38883/
-----------------------------------------------------------
(Updated Sept. 29, 2015, 11:18 p.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/38883/diff/
Testing (updated)
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan