Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[John Sirois]

> On Dec. 23, 2015, 9:48 a.m., Joshua Cohen wrote:
> > build-support/jenkins/review_feedback.py, lines 142-143
> > 
> >
> > Not sure there's anything we can do about it, but if the review has 
> > been committed but the RB was not marked as submitted we'll likely fail to 
> > apply the patch causing ReviewBot to ask if it needs to be rebased.
> > 
> > Is there potentially a better way to message that edge case? If not, 
> > it's just something we need to be aware of (stay on top of closing RB's, 
> > and keep an eye out for erroneous "needs rebase" messages from ReviewBot).
> 
> John Sirois wrote:
> If the concept of a patch chain were made explicit and passed around then 
> the messaging could fork based on "Are we in a patch chain or is this a 
> simple patch with no dependents?".
> 
> For the patch chain cases we could always mention the full patch chain in 
> the error messages, ie
> ```
> Error applying patch chain for RB#456 [master (sha) <- RB#123 <- RB#456].
> Failed to patch RB#123 on master (sha):
> ...[maybe include git apply stderr]...
> 
> RB#123 may already have been submitted but the review is not marked as 
> such.  If so - please mark RB#123 submitted.
> Otherwise you may need to rebase the patch chain.
> ```
> 
> Versus the simple cases:
> ```
> Error patching RB#789 on master (sha):
> ...[maybe include git apply stderr]...
> 
> Do you need to rebase?
> ```
> 
> Your call whether that should happen now or later.  I'd prefer later and 
> will file an issue against myself if you agree.
> 
> Joshua Cohen wrote:
> Perfectly happy to address in a follow up as time allows. I'll commit 
> this, feel free to file an issue to track the follow up.

Thanks Josh - filed https://issues.apache.org/jira/browse/AURORA-1567 to track 
this improvement.


- John


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


On Dec. 22, 2015, 2:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 2:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[Joshua Cohen]

> On Dec. 23, 2015, 4:48 p.m., Joshua Cohen wrote:
> > build-support/jenkins/review_feedback.py, lines 142-143
> > 
> >
> > Not sure there's anything we can do about it, but if the review has 
> > been committed but the RB was not marked as submitted we'll likely fail to 
> > apply the patch causing ReviewBot to ask if it needs to be rebased.
> > 
> > Is there potentially a better way to message that edge case? If not, 
> > it's just something we need to be aware of (stay on top of closing RB's, 
> > and keep an eye out for erroneous "needs rebase" messages from ReviewBot).
> 
> John Sirois wrote:
> If the concept of a patch chain were made explicit and passed around then 
> the messaging could fork based on "Are we in a patch chain or is this a 
> simple patch with no dependents?".
> 
> For the patch chain cases we could always mention the full patch chain in 
> the error messages, ie
> ```
> Error applying patch chain for RB#456 [master (sha) <- RB#123 <- RB#456].
> Failed to patch RB#123 on master (sha):
> ...[maybe include git apply stderr]...
> 
> RB#123 may already have been submitted but the review is not marked as 
> such.  If so - please mark RB#123 submitted.
> Otherwise you may need to rebase the patch chain.
> ```
> 
> Versus the simple cases:
> ```
> Error patching RB#789 on master (sha):
> ...[maybe include git apply stderr]...
> 
> Do you need to rebase?
> ```
> 
> Your call whether that should happen now or later.  I'd prefer later and 
> will file an issue against myself if you agree.

Perfectly happy to address in a follow up as time allows. I'll commit this, 
feel free to file an issue to track the follow up.


- Joshua


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


On Dec. 22, 2015, 9:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 9:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[John Sirois]

> On Dec. 23, 2015, 9:48 a.m., Joshua Cohen wrote:
> > build-support/jenkins/review_feedback.py, lines 142-143
> > 
> >
> > Not sure there's anything we can do about it, but if the review has 
> > been committed but the RB was not marked as submitted we'll likely fail to 
> > apply the patch causing ReviewBot to ask if it needs to be rebased.
> > 
> > Is there potentially a better way to message that edge case? If not, 
> > it's just something we need to be aware of (stay on top of closing RB's, 
> > and keep an eye out for erroneous "needs rebase" messages from ReviewBot).

If the concept of a patch chain were made explicit and passed around then the 
messaging could fork based on "Are we in a patch chain or is this a simple 
patch with no dependents?".

For the patch chain cases we could always mention the full patch chain in the 
error messages, ie
```
Error applying patch chain for RB#456 [master (sha) <- RB#123 <- RB#456].
Failed to patch RB#123 on master (sha):
...[maybe include git apply stderr]...

RB#123 may already have been submitted but the review is not marked as such.  
If so - please mark RB#123 submitted.
Otherwise you may need to rebase the patch chain.
```

Versus the simple cases:
```
Error patching RB#789 on master (sha):
...[maybe include git apply stderr]...

Do you need to rebase?
```

Your call whether that should happen now or later.  I'd prefer later and will 
file an issue against myself if you agree.


- John


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


On Dec. 22, 2015, 2:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 2:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On Dec. 22, 2015, 1:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 1:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[Joshua Cohen]

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

Ship it!


lgtm, thanks for adding this! Let me know what you think about the comment 
below, I'm fine to ship as-is if there's nothing to be done.


build-support/jenkins/review_feedback.py (lines 138 - 139)


Not sure there's anything we can do about it, but if the review has been 
committed but the RB was not marked as submitted we'll likely fail to apply the 
patch causing ReviewBot to ask if it needs to be rebased.

Is there potentially a better way to message that edge case? If not, it's 
just something we need to be aware of (stay on top of closing RB's, and keep an 
eye out for erroneous "needs rebase" messages from ReviewBot).


- Joshua Cohen


On Dec. 22, 2015, 9:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 9:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[Aurora ReviewBot]

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

Ship it!


Master (e75cdba) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 22, 2015, 9:34 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 9:34 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[John Sirois]

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

(Updated Dec. 22, 2015, 2:34 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Fix process to handle the no diffs case and exit early.

 build-support/jenkins/review_feedback.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)


Repository: aurora


Description
---

This adds support for following `depends-on` chains of in-flight RBs to
form patch sets ultimately based off master.

Request processing logic is factored up into a helper class that main
drives in a loop over pending RBs.

 build-support/jenkins/review_feedback.py | 246 
+-
 1 file changed, 143 insertions(+), 103 deletions(-)


Diffs (updated)
-

  build-support/jenkins/review_feedback.py 
ee37742c78a7b28bc1ccc687afae17f711471fc4 

Diff: https://reviews.apache.org/r/41659/diff/


Testing
---

Extensive testing against a local server.


Thanks,

John Sirois

Re: Review Request 41659: Support in-flight review chains via `depends-on`.

[John Sirois]

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


One assumption I used in testing this against my local server was that some 
external process ensures the `origin/master` reference is up2date (jenkins).  
In my testing that external process was me doing a fetch before running this 
tool.

- John Sirois


On Dec. 22, 2015, 2:26 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41659/
> ---
> 
> (Updated Dec. 22, 2015, 2:26 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This adds support for following `depends-on` chains of in-flight RBs to
> form patch sets ultimately based off master.
> 
> Request processing logic is factored up into a helper class that main
> drives in a loop over pending RBs.
> 
>  build-support/jenkins/review_feedback.py | 246 
> +-
>  1 file changed, 143 insertions(+), 103 deletions(-)
> 
> 
> Diffs
> -
> 
>   build-support/jenkins/review_feedback.py 
> ee37742c78a7b28bc1ccc687afae17f711471fc4 
> 
> Diff: https://reviews.apache.org/r/41659/diff/
> 
> 
> Testing
> ---
> 
> Extensive testing against a local server.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Review Request 41659: Support in-flight review chains via `depends-on`.

[John Sirois]

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

Review request for Aurora, Joshua Cohen and Bill Farner.


Repository: aurora


Description
---

This adds support for following `depends-on` chains of in-flight RBs to
form patch sets ultimately based off master.

Request processing logic is factored up into a helper class that main
drives in a loop over pending RBs.

 build-support/jenkins/review_feedback.py | 246 
+-
 1 file changed, 143 insertions(+), 103 deletions(-)


Diffs
-

  build-support/jenkins/review_feedback.py 
ee37742c78a7b28bc1ccc687afae17f711471fc4 

Diff: https://reviews.apache.org/r/41659/diff/


Testing
---

Extensive testing against a local server.


Thanks,

John Sirois