Intent to implement: "git cl try" triggers CQ dry run

[Andrii Shyshkalov]

Hi Chromium devs!

If you don't use git cl try, you can stop reading now.

tl;dr I want to make git cl try trigger CQ dry run, which would result in you getting two emails per git cl try run. WDYT?

Current state

1. git cl try loads builders from infra/config/cq.cfg which might be stale, which is particularly bad if builder removed/renamed (http://crbug.com/625697).

2. Experimental builders in cq.cfg are ignored by git cl try, which is probably OK.

3. If a user specified extra bots on command line, they would be added to the builders from cq.cfg.

4. If CL description has CQ_INCLUDE_TRYBOTS=..., these would be ignored by git cl try, and that confuses users (=developers) (http://crbug.com/585237)

5. Some projects have cq.cfg in another place, which require extra work for such projects (http://crbug.com/581150, sorry Google internal only).

Proposal

If PRESUBMIT.py specifies GetPreferredTryMasters (example), then use trybots that it returns (no change).

Else, git cl try triggers CQ dry run (~= git cl set-commit -d), and if there are extra trybots specifeid on command line schedules these builds directly.

Pros

1. Does the obvious thing for users (I think), addressing bugs above.

2. Default works correctly for most projects with CQ.

3. Still possible to override as before.

Cons

1. git cl try will now result in 2 messages on Rietveld (CQ Dry Run started and passed/failed). There is a bug about that, but it's not trivial to fix. Also, this means CL author, but not reviewers, will receive 2 emails.

Does anybody has concerns/suggestions about this?

Thanks,

Andrii

[Sadrul Chowdhury]

On Thu, Jul 7, 2016 at 10:57 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:

Hi Chromium devs!

If you don't use git cl try, you can stop reading now.

tl;dr I want to make git cl try trigger CQ dry run, which would result in you getting two emails per git cl try run. WDYT?

-1 for the extra mails. I would much prefer if we just got rid of the mails and the generated CR comments from a dry-run.

Sadrul

 

Current state

1. git cl try loads builders from infra/config/cq.cfg which might be stale, which is particularly bad if builder removed/renamed (http://crbug.com/625697).

2. Experimental builders in cq.cfg are ignored by git cl try, which is probably OK.

3. If a user specified extra bots on command line, they would be added to the builders from cq.cfg.

4. If CL description has CQ_INCLUDE_TRYBOTS=..., these would be ignored by git cl try, and that confuses users (=developers) (http://crbug.com/585237)

5. Some projects have cq.cfg in another place, which require extra work for such projects (http://crbug.com/581150, sorry Google internal only).

Proposal

If PRESUBMIT.py specifies GetPreferredTryMasters (example), then use trybots that it returns (no change).

Else, git cl try triggers CQ dry run (~= git cl set-commit -d), and if there are extra trybots specifeid on command line schedules these builds directly.

Pros

1. Does the obvious thing for users (I think), addressing bugs above.

2. Default works correctly for most projects with CQ.

3. Still possible to override as before.

Cons

1. git cl try will now result in 2 messages on Rietveld (CQ Dry Run started and passed/failed). There is a bug about that, but it's not trivial to fix. Also, this means CL author, but not reviewers, will receive 2 emails.

Does anybody has concerns/suggestions about this?

Thanks,

Andrii

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Lucas Gadani]

On Thu, Jul 7, 2016 at 11:23 AM Sadrul Chowdhury <sad...@chromium.org> wrote:

On Thu, Jul 7, 2016 at 10:57 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:

Hi Chromium devs!

If you don't use git cl try, you can stop reading now.

tl;dr I want to make git cl try trigger CQ dry run, which would result in you getting two emails per git cl try run. WDYT?

-1 for the extra mails. I would much prefer if we just got rid of the mails and the generated CR comments from a dry-run.

  I agree with Sadrul. I avoid using dry run and prefer using git cl try just to avoid the extra spam.

[Lucas Gadani]

On Thu, Jul 7, 2016 at 11:23 AM Sadrul Chowdhury <sad...@chromium.org> wrote:

On Thu, Jul 7, 2016 at 10:57 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:

Hi Chromium devs!

If you don't use git cl try, you can stop reading now.

tl;dr I want to make git cl try trigger CQ dry run, which would result in you getting two emails per git cl try run. WDYT?

-1 for the extra mails. I would much prefer if we just got rid of the mails and the generated CR comments from a dry-run.

[Brett Wilson]

I'm very happy to change this. The fact that we have to similar but different ways of doing this seems very confusing.

I don't think that we should keep two ways of doing this just because some people happen to use a try to avoid email from a dry run. It seems like a separate problem if people don't like dry run emails, and maybe we should reevaluate what we send (I would personally vote for no "CQ is trying da patch" email).It seems that email policy changes can be done asynchronously from "cl try" updates.

Brett

--

[James Cook]

Would this result in people being unable to check the "CQ" box while a dry-run is in progress?

I frequently have small patches that get a LGTM while my "git cl try" is running and I like being able to check the CQ box. I don't think I can check it during a dry run, at least one started via the UI.

James

[Christian Biesinger]

You can uncheck the box and re-check it. That will turn a dry-run into a real CQ run. No trybots will be aborted. I use this a lot.

-Christian

[Dirk Pranke]

On Thu, Jul 7, 2016 at 7:57 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:

Hi Chromium devs!

If you don't use git cl try, you can stop reading now.

tl;dr I want to make git cl try trigger CQ dry run, which would result in you getting two emails per git cl try run. WDYT?

Current state

1. git cl try loads builders from infra/config/cq.cfg which might be stale, which is particularly bad if builder removed/renamed (http://crbug.com/625697).

2. Experimental builders in cq.cfg are ignored by git cl try, which is probably OK.

3. If a user specified extra bots on command line, they would be added to the builders from cq.cfg.

4. If CL description has CQ_INCLUDE_TRYBOTS=..., these would be ignored by git cl try, and that confuses users (=developers) (http://crbug.com/585237)

5. Some projects have cq.cfg in another place, which require extra work for such projects (http://crbug.com/581150, sorry Google internal only).

Proposal

If PRESUBMIT.py specifies GetPreferredTryMasters (example), then use trybots that it returns (no change).

Else, git cl try triggers CQ dry run (~= git cl set-commit -d), and if there are extra trybots specifeid on command line schedules these builds directly.

Question: In that specific example, we'd actually want to delete GetPreferredTryMasters from chromium's PRESUBMIT.py, because it's trying to do the thing that a CQ dry run would do, but it's doing it wrong (since it's using the local config), right?

 

Pros

1. Does the obvious thing for users (I think), addressing bugs above.

2. Default works correctly for most projects with CQ.

3. Still possible to override as before.

Cons

1. git cl try will now result in 2 messages on Rietveld (CQ Dry Run started and passed/failed). There is a bug about that, but it's not trivial to fix. Also, this means CL author, but not reviewers, will receive 2 emails.

I think we should make this change -- for the same reasons Brett does -- and hopefully fix the bug :).

-- Dirk

Does anybody has concerns/suggestions about this?

Thanks,

Andrii

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/7c00721e-c545-4590-b5ec-6d9329bc5159%40chromium.org.

[Gabriel Charette]

On Thu, Jul 7, 2016 at 12:45 PM Christian Biesinger <cbies...@chromium.org> wrote:

You can uncheck the box and re-check it. That will turn a dry-run into a real CQ run. No trybots will be aborted. I use this a lot.

I use this too but if it's not obvious to everyone maybe it should be a tri-state dropdown instead of a checkbox?

[Sadrul Chowdhury]

On Thu, Jul 7, 2016 at 12:44 PM, Christian Biesinger <cbies...@chromium.org> wrote:

You can uncheck the box and re-check it. That will turn a dry-run into a real CQ run. No trybots will be aborted. I use this a lot.

Is there an equivalent workflow for 'git cl'? i.e. if 'git cl try' (or 'git cl set_commit -d') triggers a CQ dry-run, would 'git cl set_commit' put it in the CQ?

Sadrul

 

-Christian

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[James Cook]

Or maybe the CQ checkbox just shouldn't be checked during dry-runs. The text next to it could say "dry run in progress" instead of the "dry run" it says now.

[Steve Kobes]

Rietveld's "new UI" (Settings > uncheck "Deprecated ui"), instead of a checkbox, has buttons labeled Commit / Cancel Commit / Start Dry Run / Cancel Commit Dry Run, which is the right idea IMO.  It's missing a one-click dry run -> commit, but that's probably an easy fix.

--

You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAKLzqjGyLCPqC%3DSB0%2BP_zZm35n6cgVHFdGjkGGAu2%2BY2Mm_x5g%40mail.gmail.com.

[Andrii Shyshkalov]

Thanks to everyone for replies. Here is what I've done in the meantime thanks for to your feedback:

1. Addressed CQ Dry run message pollution (bug) (pending deployment to production). These messages would now be hidden behind "Show Generated Messages".

  Pending review & deployment is also showing "CQ Status" url next to commit box, so you don't have to search for it in messages.

2. I propose to avoid sending email to anyone on CQ dry run messages (bug).

  In the meantime, if you don't like CQ Dry run emails, this bug has suggestions on filter set up.

Answers to a few related questions asked:

* git cl set-commit indeed triggers CQ for full run and commit. So does git cl upload -c|--use-commit-queue. See help of respective commands for more info.

* there is no reason to wait for CQ dry run to finish if you are ready to actually commit, cancel dry run and start full run, as others have said.

> Question: In that specific example, we'd actually want to delete GetPreferredTryMasters from chromium's PRESUBMIT.py, because it's trying to do the thing that a CQ dry run would do, but it's doing it wrong (since it's using the local config), right?

Yes, absolutely correct.

 

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.

[Lucas Gadani]

Thanks for addressing the feedback! I have one more question: will I still be able to use 'git cl try -m master -b bot' after the proposed changes if I want to run through a single/specific bot?

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAKLzqjGyLCPqC%3DSB0%2BP_zZm35n6cgVHFdGjkGGAu2%2BY2Mm_x5g%40mail.gmail.com.

[Brett Wilson]

This all sounds good to me!

Brett

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

[daniel Amsalem]

Hi,

Please do not send me e-mails, it's a mistake I do not know you, thank you,

Daniel

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Sergiy]

On Thursday, July 7, 2016 at 9:38:03 PM UTC+2, Andrii Shyshkalov wrote:

> Question: In that specific example, we'd actually want to delete GetPreferredTryMasters from chromium's PRESUBMIT.py, because it's trying to do the thing that a CQ dry run would do, but it's doing it wrong (since it's using the local config), right?

Yes, absolutely correct.

There is even a bug for this: http://crbug.com/577224.

[Andrii Shyshkalov]

Update:

  Previously, I proposed to avoid sending email to anyone on CQ dry run messages (bug). This was not quite correct. Thanks for rmistry@ for letting me know.

  The first message "Dry Run: CQ trying da patch" won't result in any email, as it isn't useful.

  The second message whether Dry Run passed or failed would still be sent.

On Thursday, July 7, 2016 at 9:42:59 PM UTC+2, Lucas Gadani wrote:

Thanks for addressing the feedback! I have one more question: will I still be able to use 'git cl try -m master -b bot' after the proposed changes if I want to run through a single/specific bot?

Correct. Specifying bots manually will trigger them directly and only them.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAKLzqjGyLCPqC%3DSB0%2BP_zZm35n6cgVHFdGjkGGAu2%2BY2Mm_x5g%40mail.gmail.com.

[Alan Cutter]

Is there a tracking bug where we can check the status of this "git cl try" change?

[Andrii Shyshkalov]

Updates: CQ Dry runs now send email only to the person who triggered it, and author if different (http://crbug.com/596249).

Next: actually proposed change of git cl try tracked in http://crbug.com/625697 

On Monday, July 11, 2016 at 2:52:37 AM UTC+2, Alan Cutter wrote:

Is there a tracking bug where we can check the status of this "git cl try" change?

http://crbug.com/625697 

[Andrii Shyshkalov]

And all this has just been completed and available for you after next "gclient sync" or manual depot_tools update.

[Trent Apted]

On 8 July 2016 at 19:59, Andrii Shyshkalov <tan...@chromium.org> wrote:

  The second message whether Dry Run passed or failed would still be sent.

PSA: This now happens.

So when commit-bot sends you an email after `git cl try` on an unpublished and definitely-not-ready CL, you didn't *actually* try to commit it by accident. No need to have a mini heart attack like I did :p.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAKLzqjGyLCPqC%3DSB0%2BP_zZm35n6cgVHFdGjkGGAu2%2BY2Mm_x5g%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--

[Scott Violet]

Is there a bug filed to make it more obvious how to convert a try into a commit?

Here's the current flow:

> upload patch
> git cl try
codereview now shows a check next to Commit
> get approvals
Click commit button, toggling to uncommit (confusing)
Click commit button again

It's those last two that are confusing and could really use a better ui.

-Scott

On Thu, Jul 7, 2016 at 7:57 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:

> --
> You received this message because you are subscribed to the Google Groups
> "infra-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to infra-dev+...@chromium.org.
> To post to this group, send email to infr...@chromium.org.
> To view this discussion on the web visit

> https://groups.google.com/a/chromium.org/d/msgid/infra-dev/7c00721e-c545-4590-b5ec-6d9329bc5159%40chromium.org.

[dan...@chromium.org]

On Wed, Jul 20, 2016 at 9:55 AM, Scott Violet <s...@chromium.org> wrote:

Is there a bug filed to make it more obvious how to convert a try into a commit?

Here's the current flow:

> upload patch
> git cl try
codereview now shows a check next to Commit
> get approvals
Click commit button, toggling to uncommit (confusing)
Click commit button again

It's those last two that are confusing and could really use a better ui.

Definitely. But FWIW you can also "git cl set_commit" to commit.

 

[Andrii Shyshkalov]

On Wednesday, July 20, 2016 at 9:09:18 PM UTC+2, dan...@chromium.org wrote:

On Wed, Jul 20, 2016 at 9:55 AM, Scott Violet <s...@chromium.org> wrote:

Is there a bug filed to make it more obvious how to convert a try into a commit?

Here's the current flow:

> upload patch
> git cl try
codereview now shows a check next to Commit
> get approvals
Click commit button, toggling to uncommit (confusing)
Click commit button again

It's those last two that are confusing and could really use a better ui.

Definitely. But FWIW you can also "git cl set_commit" to commit.

Yep. As you might have seen our email about move towards Gerrit codereview, instead of Rietveld, we already have a better UI there. 

 

> email to infra-dev+unsubscribe@chromium.org.

> To post to this group, send email to infr...@chromium.org.
> To view this discussion on the web visit

[Scott Violet]

On Thu, Jul 21, 2016 at 6:56 AM, Andrii Shyshkalov <tan...@chromium.org> wrote:
>
>
> On Wednesday, July 20, 2016 at 9:09:18 PM UTC+2, dan...@chromium.org wrote:
>>
>> On Wed, Jul 20, 2016 at 9:55 AM, Scott Violet <s...@chromium.org> wrote:
>>>
>>> Is there a bug filed to make it more obvious how to convert a try into a
>>> commit?
>>>
>>> Here's the current flow:
>>>
>>> > upload patch
>>> > git cl try
>>> codereview now shows a check next to Commit
>>> > get approvals
>>> Click commit button, toggling to uncommit (confusing)
>>> Click commit button again
>>>
>>> It's those last two that are confusing and could really use a better ui.
>>
>>
>> Definitely. But FWIW you can also "git cl set_commit" to commit.
>
> Yep. As you might have seen our email about move towards Gerrit codereview,
> instead of Rietveld, we already have a better UI there.

Correct me if I'm wrong, but isn't the move at least more than a
quarter away? Assuming that's the case, we should fix this now as the
current ui is in a confusing and misleading state. If the answer is
no, wait for Gerrit, then why did we attempt to change things at all?

-Scott

>>> > email to infra-dev+...@chromium.org.