mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 43552: Added a support/push-reviews.py script to push reviews upstream.
Date Mon, 22 Feb 2016 04:53:28 GMT


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > In general, I prefer scripts with a bunch of helper functions and a compact main()
that steps through each of them. I'm not sure what the general concensus for the Mesos code
base is, but I generally find this easier to walk through.  Example: support/generate-endpoint-help.py

In our C++ code base we generally prefer procedural style as it requires fewer context switches.
We haven't officially defined the style for Python code in our code base, but I would guess
we want to be similar. 

That said if something makes sense to be a function (e.g, reuse) we should extract it into
a function. I've added a main and refactored a bit. Let me know if it looks ok.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 27-30
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line27>
> >
> >     Do we have guidelines for indentation here?  When writing the support/generate-endpoints-help.py
script, bmahler suggested something different than what you have here.

I'm trying to be close to our C++ style. This is how it is in apply-reviews.py too. 

What did bmahler suggest?


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 32-39
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line32>
> >
> >     If the tracking branch is reauired to be of the form `remote/branch` (as mentioned
in a later comment), what do we need the `remote` flag for?

"tracking branch" is mainly required to figure out the new commits to push. Technically it
can be just "master". "remote" is to figure out the upstream to push to. If remote is not
specified, we try to deduce it from "tracking branch", in which case we expect it to be a
"remote tracking branch". 

But I agree it's all confusing. I will kill both "--remote" and "--tracking_branch" and expect
users to run this from "master" branch for simplicity.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, line 52
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line52>
> >
> >     We need some sort of way to enforce that the tracking branch is always of the
form `remote/branch`. Otherwise, if we run e.g.
> >     
> >     ```
> >     $ klueska@c99:~/projects/mesos$ support/push-reviews.py -t master
> >     ```
> >     
> >     remote gets set to `master` here, which obviously breaks things later on.

see above.


> On Feb. 17, 2016, 7:02 p.m., Kevin Klues wrote:
> > support/push-reviews.py, lines 108-112
> > <https://reviews.apache.org/r/43552/diff/1/?file=1252228#file1252228line108>
> >
> >     As we close the review, we should also post a comment to reviewboard with the
commit message we actually pushed to master.
> >     
> >     You can use the --description flag:
> >     
> >     https://www.reviewboard.org/docs/rbtools/dev/rbt/commands/close/

I'll make a TODO for now as committers don't currently/always set the commit message on RB.


- Vinod


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


On Feb. 22, 2016, 4:52 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43552/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2016, 4:52 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Kevin Klues, and Michael Park.
> 
> 
> Bugs: MESOS-3929
>     https://issues.apache.org/jira/browse/MESOS-3929
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This script allows committers to push locally applied review chain to the ASF
> git repo and mark the reviews as submitted.
> 
> 
> Diffs
> -----
> 
>   support/push-reviews.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43552/diff/
> 
> 
> Testing
> -------
> 
> Tested locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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