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/24 06:26:53 UTC
Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
Review request for mesos, 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
-------
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100770
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705]
All tests passed.
- Mesos ReviewBot
On Sept. 27, 2015, 2:02 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2015, 2:02 a.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Guangya Liu <gy...@gmail.com>.
> On 十月 1, 2015, 1:12 p.m., Guangya Liu wrote:
> > support/apply-reviews.py, line 2
> > <https://reviews.apache.org/r/38705/diff/3/?file=1087670#file1087670line2>
> >
> > The import should be in alpha order
>
> Artem Harutyunyan wrote:
> I am happy to fix this, but could you please justify your reasoning?
Please check https://github.com/apache/mesos/blob/master/support/post-reviews.py#L25-#L30
Also I see that most of the import or include are using alpha order in mesos.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101225
-----------------------------------------------------------
On 十月 1, 2015, 5:12 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated 十月 1, 2015, 5:12 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 1, 2015, 6:12 a.m., Guangya Liu wrote:
> > support/apply-reviews.py, line 2
> > <https://reviews.apache.org/r/38705/diff/3/?file=1087670#file1087670line2>
> >
> > The import should be in alpha order
I am happy to fix this, but could you please justify your reasoning?
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101225
-----------------------------------------------------------
On Sept. 29, 2015, 11:09 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 11:09 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 1, 2015, 6:12 a.m., Guangya Liu wrote:
> > support/apply-reviews.py, line 2
> > <https://reviews.apache.org/r/38705/diff/3/?file=1087670#file1087670line2>
> >
> > The import should be in alpha order
>
> Artem Harutyunyan wrote:
> I am happy to fix this, but could you please justify your reasoning?
>
> Guangya Liu wrote:
> Please check https://github.com/apache/mesos/blob/master/support/post-reviews.py#L25-#L30
>
> Also I see that most of the import or include are using alpha order in mesos.
Great! I fixed the order. Thanks for pointing this out!
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101225
-----------------------------------------------------------
On Oct. 8, 2015, 11:26 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 11:26 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101225
-----------------------------------------------------------
support/apply-reviews.py (line 2)
<https://reviews.apache.org/r/38705/#comment158599>
The import should be in alpha order
- Guangya Liu
On 九月 30, 2015, 6:09 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated 九月 30, 2015, 6:09 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 3, 2015, 7:46 a.m., haosdent huang wrote:
> > IMHO, we keep both apply-reviews.py and apply-reviews.sh and call apply-reviews.sh in apply-reviews.py looks strange. It would be better we add this function to apply-review.sh. Or create apply-reviews.py and deprecated apply-reviews.sh
Please take a look at the followup review (https://reviews.apache.org/r/38883/).
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101405
-----------------------------------------------------------
On Oct. 8, 2015, 11:26 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 11:26 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101405
-----------------------------------------------------------
IMHO, we keep both apply-reviews.py and apply-reviews.sh and call apply-reviews.sh in apply-reviews.py looks strange. It would be better we add this function to apply-review.sh. Or create apply-reviews.py and deprecated apply-reviews.sh
- haosdent huang
On Oct. 1, 2015, 5:12 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2015, 5:12 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review102877
-----------------------------------------------------------
Thanks for all the tips, Marco\! I learned a lot of Python here :).
- Artem Harutyunyan
On Oct. 15, 2015, 11:50 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2015, 11:50 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 22, 2015, 9:20 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 25
> > <https://reviews.apache.org/r/38705/diff/8-9/?file=1100640#file1100640line25>
> >
> > s/extract_//
>
> Artem Harutyunyan wrote:
> Marco commented earlier on this one `nit: you are 'masking' the global builtin id() here - that's a PEP8 violation, could you please rename to something like rid?`.
is the concern that if you call this method review_id(), you cannot use "review_id" as a variable in the method? if yes, you can call the local variable 'rid'.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review103656
-----------------------------------------------------------
On Oct. 23, 2015, 6:16 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 6:16 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 22, 2015, 2:20 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 25
> > <https://reviews.apache.org/r/38705/diff/8-9/?file=1100640#file1100640line25>
> >
> > s/extract_//
Marco commented earlier on this one `nit: you are 'masking' the global builtin id() here - that's a PEP8 violation, could you please rename to something like rid?`.
> On Oct. 22, 2015, 2:20 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, line 32
> > <https://reviews.apache.org/r/38705/diff/8-9/?file=1100640#file1100640line32>
> >
> > s/parent_review/review_chain/ ?
That's exactly what I renamed that function to two reviews down the chain.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review103656
-----------------------------------------------------------
On Oct. 22, 2015, 11:16 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2015, 11:16 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review103656
-----------------------------------------------------------
support/apply-reviews.py (line 25)
<https://reviews.apache.org/r/38705/#comment161714>
s/extract_//
support/apply-reviews.py (line 32)
<https://reviews.apache.org/r/38705/#comment161713>
s/parent_review/review_chain/ ?
support/apply-reviews.py (lines 41 - 42)
<https://reviews.apache.org/r/38705/#comment161715>
Shouldn't this comment be rephrased/killed since you are disallowing more than one immediate parent below?
support/apply-reviews.py (line 56)
<https://reviews.apache.org/r/38705/#comment161717>
why extract summary into a variable here? you didn't do that above in #51.
support/apply-reviews.py (line 57)
<https://reviews.apache.org/r/38705/#comment161716>
what if there is a circular dependency of reviews by mistake?
A --> B --> C --> A
support/apply-reviews.py (line 58)
<https://reviews.apache.org/r/38705/#comment161718>
Why do you need to take 'r_list' as an argument to this method? to catch circular dependencies? if not, you could've done something as follows?
```
def review_list(review_id):
review = review_json(review_id)
if len (review.depends) == 0 :
return [review_id , review.summary]
elif len (review.depends) == 1:
return review_list(review.depends.id) + [review_id, review.summary]
else:
error()
```
- Vinod Kone
On Oct. 20, 2015, 7:28 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 7:28 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review104090
-----------------------------------------------------------
Note: considering the amount of changes since my last review, my "Ship It" is no longer valid.
support/apply-reviews.py (line 15)
<https://reviews.apache.org/r/38705/#comment162307>
This won't work on Windows, since it'll construct "URL"s with (evil) backslashes.
(And considering how often we use this script for the CMake/Windows review chains... :)
This would be better:
https://docs.python.org/2/library/urlparse.html#urlparse.urljoin
support/apply-reviews.py (line 38)
<https://reviews.apache.org/r/38705/#comment162310>
Nit: Extra space after the if.
support/apply-reviews.py (line 78)
<https://reviews.apache.org/r/38705/#comment162313>
Nit: Extra space after function name.
- Joseph Wu
On Oct. 22, 2015, 11:16 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2015, 11:16 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review104105
-----------------------------------------------------------
Ship it!
modulo joseph's comments.
support/apply-reviews.py (line 57)
<https://reviews.apache.org/r/38705/#comment162328>
log the id of the violating review?
- Vinod Kone
On Oct. 23, 2015, 6:16 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 6:16 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 2:52 a.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
More 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 12:28 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review104732
-----------------------------------------------------------
Ship it!
support/apply-reviews.py (line 45)
<https://reviews.apache.org/r/38705/#comment162983>
You could add a newline at the end of this string. (`write` doesn't do that for you.)
support/apply-reviews.py (line 57)
<https://reviews.apache.org/r/38705/#comment162984>
Newline at end of string.
- Joseph Wu
On Oct. 28, 2015, 2:57 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2015, 2:57 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 28, 2015, 2:57 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 22, 2015, 11:16 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 12:28 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 19, 2015, 1:04 p.m., Vinod Kone wrote:
> > support/apply-reviews.py, lines 49-52
> > <https://reviews.apache.org/r/38705/diff/8/?file=1100640#file1100640line49>
> >
> > i don't follow what's happening here. you are appending the same <rid,summary> pair multiple times to the list?
Not exactly, but I refactored this part so it should be clear now.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review103147
-----------------------------------------------------------
On Oct. 18, 2015, 3:30 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2015, 3:30 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review103147
-----------------------------------------------------------
support/apply-reviews.py (line 12)
<https://reviews.apache.org/r/38705/#comment161109>
s/rid/review_id/
here and everywhere else.
support/apply-reviews.py (line 22)
<https://reviews.apache.org/r/38705/#comment161104>
why not have this return JSON instead of string?
support/apply-reviews.py (line 25)
<https://reviews.apache.org/r/38705/#comment161107>
s/extract/review/
support/apply-reviews.py (line 32)
<https://reviews.apache.org/r/38705/#comment161110>
s/r_list/reviews/
also, the function name is misleading. how about
s/parent_review/review_chain/ ?
support/apply-reviews.py (lines 33 - 34)
<https://reviews.apache.org/r/38705/#comment161111>
what does "reversed chain of review requests for a given Review ID" mean? i'm assuming you mean dependent reviews? please make that more clear.
support/apply-reviews.py (lines 35 - 36)
<https://reviews.apache.org/r/38705/#comment161105>
if review_json returns json, this could be
json = review_json(review_url(rid))
support/apply-reviews.py (line 43)
<https://reviews.apache.org/r/38705/#comment161106>
s/A may/A review may/
do we want to allow reviews that have more than one parent review? what does that mean for the order of reviews to be applied? i think it makes sense to disallow such reviews. throw an error.
support/apply-reviews.py (lines 49 - 52)
<https://reviews.apache.org/r/38705/#comment161113>
i don't follow what's happening here. you are appending the same <rid,summary> pair multiple times to the list?
support/apply-reviews.py (line 56)
<https://reviews.apache.org/r/38705/#comment161093>
2 lines.
support/apply-reviews.py (line 75)
<https://reviews.apache.org/r/38705/#comment161103>
seems weird to have this function return the command if dry_run is true and None otherwise.
how about just prefixing "echo" to the command if dry_run is true?
support/apply-reviews.py (line 101)
<https://reviews.apache.org/r/38705/#comment161108>
s/print //
- Vinod Kone
On Oct. 18, 2015, 10:30 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2015, 10:30 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 18, 2015, 3:30 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 17, 2015, 7:37 p.m.)
Review request for mesos, Benjamin Hindman, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Bugfix and 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 15, 2015, 11:50 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Oct. 14, 2015, 5:55 a.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.
`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`.
> On Oct. 14, 2015, 5:55 a.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?
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?
> On Oct. 14, 2015, 5:55 a.m., Marco Massenzio wrote:
> > support/apply-reviews.py, line 24
> > <https://reviews.apache.org/r/38705/diff/5/?file=1093292#file1093292line24>
> >
> > if you are doing this often enough, it's much better to compile this to a constant Pattern and then use that instead.
It's done only once per review, so compiling will not give us any significant performance increase.
> On Oct. 14, 2015, 5:55 a.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`?
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)).
> On Oct. 14, 2015, 5:55 a.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`)
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.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101980
-----------------------------------------------------------
On Oct. 8, 2015, 11:26 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 11:26 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101980
-----------------------------------------------------------
support/apply-reviews.py (line 7)
<https://reviews.apache.org/r/38705/#comment159488>
uhm... could we use `requests` instead?
much more modern API and widespread use.
support/apply-reviews.py (line 11)
<https://reviews.apache.org/r/38705/#comment159484>
nit: you are 'masking' the global builtin `id()` here - that's a PEP8 violation, could you please rename to something like `rid`?
support/apply-reviews.py (line 13)
<https://reviews.apache.org/r/38705/#comment159486>
don't concatenate string, prefer the use of os.path.join():
```
import os
os.path.join(REVIEW_URL, rid)
```
(why do you need the trailing slash?)
support/apply-reviews.py (line 19)
<https://reviews.apache.org/r/38705/#comment159489>
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?
support/apply-reviews.py (line 24)
<https://reviews.apache.org/r/38705/#comment159490>
if you are doing this often enough, it's much better to compile this to a constant Pattern and then use that instead.
support/apply-reviews.py (line 29)
<https://reviews.apache.org/r/38705/#comment159495>
you should not call a param with the name of a type (`list` is a type in Python).
also, it's bad practice to initialize it as you do there.
BTW - can you please use the @param to explain what the method expects?
Prefer:
```
def parent_review(rid, revs_list=None):
"""Returns a list with reversed chain of review requests for a given
Review ID.
"""
result = revs_list or []
json_str = review_json(review_url(rid))
json_obj = json.loads(json_str)
# Stop as soon as we stumble upon a submitted request.
rr = json_obj.get('review_request')
if rr and rr.get('status') == "submitted":
return result
for deps in rr.get('summary'):
...
```
Also, worth always using `get()` instead of accessor (`[]`) for data that you are not sure may or may not be there (it's an API response after all) - the latter will throw a KeyError if the key is missing; `get` will just give you back None (or you can pass a second optional default return value if you want).
support/apply-reviews.py (line 40)
<https://reviews.apache.org/r/38705/#comment159497>
is this a recursion inside an iteration?
can you please add a comment? I'm sure folks who may one day try to fix/augment this script will be just as confused...
support/apply-reviews.py (line 45)
<https://reviews.apache.org/r/38705/#comment159496>
is this right?
you return after the first iteration?
if so, why not just getting the first item in `depends_on`?
support/apply-reviews.py (line 56)
<https://reviews.apache.org/r/38705/#comment159499>
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`)
support/apply-reviews.py (line 90)
<https://reviews.apache.org/r/38705/#comment159501>
nit: `if r not in applied:` is more pythonic :)
(also, PEP8 doesn't really like 1- and 2-letter variables)
support/apply-reviews.py (line 91)
<https://reviews.apache.org/r/38705/#comment159502>
do you need a counter or something like that, or a flag would suffice?
I'm not sure how you use `applied` as a dict (looks like you are just using it as a `set`?):
```
applied = set()
for review, summary in reviews:
if review not in applied:
applied.add(review)
print(apply_review(...))
```
- Marco Massenzio
On Oct. 8, 2015, 6:26 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2015, 6:26 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 8, 2015, 11:26 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/38705/diff/
Testing
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Oct. 1, 2015, 10:12 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 (updated)
-----
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
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review101129
-----------------------------------------------------------
Ship it!
LGTM!
Note: The long-form cmd-line arguments (i.e. `--dry-run`) are not really necessary, but might be nice.
support/apply-reviews.py (line 12)
<https://reviews.apache.org/r/38705/#comment158465>
Typo: Retutns
- Joseph Wu
On Sept. 29, 2015, 11:09 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 11:09 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/38705/diff/
>
>
> Testing
> -------
>
> Tested the script with python 2.7.
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Sept. 29, 2015, 11:09 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/38705/diff/
Testing (updated)
-------
Tested the script with python 2.7.
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100823
-----------------------------------------------------------
Ship it!
support/apply-reviews.py (line 8)
<https://reviews.apache.org/r/38705/#comment158071>
Not used.
support/apply-reviews.py (line 12)
<https://reviews.apache.org/r/38705/#comment158069>
Typo: Retutns.
Also, docstrings go inside the method body.
```
def review_url(id):
"""Returns a Review Board URL given a review ID."""
```
You can tell if it is correct if you do something like:
```
python
import apply-reviews
print review_url.__doc__
```
support/apply-reviews.py (line 18)
<https://reviews.apache.org/r/38705/#comment158070>
s/JSON-ifyed/JSON-ified/
- Joseph Wu
On Sept. 26, 2015, 7:02 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2015, 7:02 p.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/
-----------------------------------------------------------
(Updated Sept. 26, 2015, 7:02 p.m.)
Review request for mesos, Joris Van Remoortere, Joseph Wu, and Vinod Kone.
Changes
-------
Addressed comments; changed tab size to 2 spaces.
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/38705/diff/
Testing
-------
Thanks,
Artem Harutyunyan
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Sept. 24, 2015, 5:38 p.m., Joseph Wu wrote:
> > support/apply-reviews.py, line 13
> > <https://reviews.apache.org/r/38705/diff/1/?file=1083811#file1083811line13>
> >
> > Might be cleaner/safer to use urlparse.urljoin for this:
> > https://docs.python.org/2/library/urlparse.html#urlparse.urljoin
`urlparse.urljoin` does not seem to do exactly what is needed here.
> On Sept. 24, 2015, 5:38 p.m., Joseph Wu wrote:
> > support/apply-reviews.py, line 68
> > <https://reviews.apache.org/r/38705/diff/1/?file=1083811#file1083811line68>
> >
> > Seems like you don't need a `-r` in this case, because we'd only use this script for reviewboard.
> >
> > i.e. s/-r/review/
I have it for compatibility with `apply-review.sh` and also in case we decide to add support for github later.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100505
-----------------------------------------------------------
On Sept. 23, 2015, 9:26 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:26 p.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100505
-----------------------------------------------------------
Mostly just python style and commenting. My python is a bit rusty though.
Could you also mention in "Testing Done" whether this is python 2 or 3 or both?
support/apply-reviews.py (line 9)
<https://reviews.apache.org/r/38705/#comment157711>
Spaces around `=`?
support/apply-reviews.py (line 11)
<https://reviews.apache.org/r/38705/#comment157713>
Could you change these method comments to python docstrings comments?
i.e.
```
def review_url(id):
"""Returns the Review Board URL for the given review ID."""
```
support/apply-reviews.py (line 13)
<https://reviews.apache.org/r/38705/#comment157712>
Might be cleaner/safer to use urlparse.urljoin for this:
https://docs.python.org/2/library/urlparse.html#urlparse.urljoin
support/apply-reviews.py (line 15)
<https://reviews.apache.org/r/38705/#comment157714>
Docstring-ify.
Looks like this method is just json-ifying an HTTP GET from the url. (Reword comment?)
support/apply-reviews.py (line 20)
<https://reviews.apache.org/r/38705/#comment157715>
Docstring-ify.
support/apply-reviews.py (lines 26 - 28)
<https://reviews.apache.org/r/38705/#comment157716>
Docstring-ify.
Is the list of reviews in reverse order of the chain?
support/apply-reviews.py (line 29)
<https://reviews.apache.org/r/38705/#comment157720>
I believe the python convention for (not-really) "private" helper methods is a leading underscore.
i.e. `def _parent_review(...)`
support/apply-reviews.py (line 40)
<https://reviews.apache.org/r/38705/#comment157721>
Extra newline here.
support/apply-reviews.py (lines 44 - 45)
<https://reviews.apache.org/r/38705/#comment157717>
Docstring-ify.
support/apply-reviews.py (line 46)
<https://reviews.apache.org/r/38705/#comment157722>
Seems like you could combine this into `parent_review_ex` by changing the function prototype to:
`parent_review(id, list=[])`
support/apply-reviews.py (line 49)
<https://reviews.apache.org/r/38705/#comment157718>
Docstring-ify.
support/apply-reviews.py (line 55)
<https://reviews.apache.org/r/38705/#comment157719>
Docstring-ify.
support/apply-reviews.py (line 56)
<https://reviews.apache.org/r/38705/#comment157725>
It would be great if this included a snippet of the review's summary.
support/apply-reviews.py (line 68)
<https://reviews.apache.org/r/38705/#comment157724>
Seems like you don't need a `-r` in this case, because we'd only use this script for reviewboard.
i.e. s/-r/review/
support/apply-reviews.py (line 72)
<https://reviews.apache.org/r/38705/#comment157723>
Add `'--dry-run'` to this option?
- Joseph Wu
On Sept. 23, 2015, 9:26 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:26 p.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100375
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38705]
All tests passed.
- Mesos ReviewBot
On Sept. 24, 2015, 4:26 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 4:26 a.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 25, 2015, 4:35 p.m., Joseph Wu wrote:
> > One more thing: when the patch fails to apply, I get this message:
> >
> > ```
> > Traceback (most recent call last):
> > File "support/apply-reviews.py", line 86, in <module>
> > apply_review(r, dry_run)
> > File "support/apply-reviews.py", line 60, in apply_review
> > shell(cmd)
> > File "support/apply-reviews.py", line 53, in shell
> > command, stderr=subprocess.STDOUT, shell=True)
> > File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 573, in check_output
> > raise CalledProcessError(retcode, cmd, output=output)
> > subprocess.CalledProcessError: Command './support/apply-review.sh -n -r 38752' returned non-zero exit status 1
> > ```
> >
> > Instead of this one:
> > ```
> > 2015-09-25 09:34:05 URL:https://reviews.apache.org/r/38752/diff/raw/ [1426/1426] -> "38752.patch" [1]
> > error: patch failed: 3rdparty/libprocess/3rdparty/CMakeLists.txt:87
> > error: 3rdparty/libprocess/3rdparty/CMakeLists.txt: patch does not apply
> > error: patch failed: cmake/MesosConfigure.cmake:114
> > error: cmake/MesosConfigure.cmake: patch does not apply
> > Failed to apply patch
> > ```
what is support/apply-reviews.py? don't think its committed yet.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100588
-----------------------------------------------------------
On Sept. 24, 2015, 4:26 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 4:26 a.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 25, 2015, 9:35 a.m., Joseph Wu wrote:
> > One more thing: when the patch fails to apply, I get this message:
> >
> > ```
> > Traceback (most recent call last):
> > File "support/apply-reviews.py", line 86, in <module>
> > apply_review(r, dry_run)
> > File "support/apply-reviews.py", line 60, in apply_review
> > shell(cmd)
> > File "support/apply-reviews.py", line 53, in shell
> > command, stderr=subprocess.STDOUT, shell=True)
> > File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 573, in check_output
> > raise CalledProcessError(retcode, cmd, output=output)
> > subprocess.CalledProcessError: Command './support/apply-review.sh -n -r 38752' returned non-zero exit status 1
> > ```
> >
> > Instead of this one:
> > ```
> > 2015-09-25 09:34:05 URL:https://reviews.apache.org/r/38752/diff/raw/ [1426/1426] -> "38752.patch" [1]
> > error: patch failed: 3rdparty/libprocess/3rdparty/CMakeLists.txt:87
> > error: 3rdparty/libprocess/3rdparty/CMakeLists.txt: patch does not apply
> > error: patch failed: cmake/MesosConfigure.cmake:114
> > error: cmake/MesosConfigure.cmake: patch does not apply
> > Failed to apply patch
> > ```
>
> Vinod Kone wrote:
> what is support/apply-reviews.py? don't think its committed yet.
I used the `apply-reviews.py` from this review (applied normally via `apply-review.sh`) to apply a separate chain (which was ~15 reviews long).
(Meta-apply :)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100588
-----------------------------------------------------------
On Sept. 23, 2015, 9:26 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:26 p.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 38705: Added support for applying a review chain
(apply-reviews.py).
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38705/#review100588
-----------------------------------------------------------
One more thing: when the patch fails to apply, I get this message:
```
Traceback (most recent call last):
File "support/apply-reviews.py", line 86, in <module>
apply_review(r, dry_run)
File "support/apply-reviews.py", line 60, in apply_review
shell(cmd)
File "support/apply-reviews.py", line 53, in shell
command, stderr=subprocess.STDOUT, shell=True)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 573, in check_output
raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command './support/apply-review.sh -n -r 38752' returned non-zero exit status 1
```
Instead of this one:
```
2015-09-25 09:34:05 URL:https://reviews.apache.org/r/38752/diff/raw/ [1426/1426] -> "38752.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/CMakeLists.txt:87
error: 3rdparty/libprocess/3rdparty/CMakeLists.txt: patch does not apply
error: patch failed: cmake/MesosConfigure.cmake:114
error: cmake/MesosConfigure.cmake: patch does not apply
Failed to apply patch
```
- Joseph Wu
On Sept. 23, 2015, 9:26 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38705/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:26 p.m.)
>
>
> Review request for mesos, 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
> -------
>
>
> Thanks,
>
> Artem Harutyunyan
>
>