add CANONICAL_URL in codereview.settings

add CANONICAL_URL in codereview.settings

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270741

[sheyang, 2014-05-13 17:54:46]

Set CANONICAL_URL for chromium project. This url is from chromium.json.

[Sergey Berezin, 2014-05-13 20:11:22]

https://codereview.chromium.org/283813002/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/283813002/diff/1/codereview.settings#newcode11
codereview.settings:11: CANONICAL_URL: svn://svn.chromium.org/chrome/trunk/src
We are transitioning to git, so we should use git base URL:
https://chromium.googlesource.com/chromium/src

[sheyang, 2014-05-13 20:24:44]

https://codereview.chromium.org/283813002/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/283813002/diff/1/codereview.settings#newcode11
codereview.settings:11: CANONICAL_URL: svn://svn.chromium.org/chrome/trunk/src
On 2014/05/13 20:11:23, Sergey Berezin wrote:
> We are transitioning to git, so we should use git base URL:
> https://chromium.googlesource.com/chromium/src

Done.

[Sergey Berezin, 2014-05-13 20:30:10]

LGTM. Though you still need a committer's stamp to land it.

[iannucci, 2014-05-14 19:21:05]

lgtm

[sheyang, 2014-05-14 19:59:16]

The CQ bit was checked by sheyang@chromium.org

[commit-bot: I haz the power, 2014-05-14 20:00:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sheyang@chromium.org/283813002/20001

[sheyang, 2014-05-14 21:52:57]

The CQ bit was unchecked by sheyang@chromium.org

[sheyang, 2014-05-14 21:55:12]

Hi Ben and Darin,

This file codereview.settings requires lgtm from OWNERS. Would you please take a
look at this CL when you have time? In addition, do you think it appropriate to
set codereview.settings as not required lgtm from OWNERS?

Thanks,

Sheng

[darin (slow to review), 2014-05-15 06:55:27]

LGTM

[sheyang, 2014-05-15 17:13:19]

The CQ bit was checked by sheyang@chromium.org

[commit-bot: I haz the power, 2014-05-15 17:13:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sheyang@chromium.org/283813002/20001

[commit-bot: I haz the power, 2014-05-15 18:21:50]

Change committed as 270741

[cmp, 2014-05-15 21:20:00]

On 2014/05/15 18:21:50, I haz the power (commit-bot) wrote:
> Change committed as 270741

This change broke the Chrome CQ.  Sergey has a fix out for it now
(http://crbug.com/373937) and I expect that fix to be live soon, so there's
probably nothing to do here such as reverting this change.

However, Sheng, we should treat this as an outage.  There are a couple problems
I see here:

- There is no BUG id for this CL, please ensure future CLs you post reference a
filed bug

- There was no TESTS line which suggests you did not test this patch before
landing it.

- If there were no manual tests, it's possible there were automated tests, but I
presume that we don't have automated tests to catch this.  What automated tests
are we missing that would have verified the codereview.settings file is valid so
in the future someone won't manually change this line again and cause the same
error?

Can you write a short postmortem that captures the essential parts and includes
some practical ideas that, if implemented, would have caught this prior to
landing in the repo (preferably in a testing phase of writing the commit).

[sheyang, 2014-05-15 21:32:59]

On 2014/05/15 21:20:00, cmp wrote:
> On 2014/05/15 18:21:50, I haz the power (commit-bot) wrote:
> > Change committed as 270741
> 
> This change broke the Chrome CQ.  Sergey has a fix out for it now
> (http://crbug.com/373937) and I expect that fix to be live soon, so there's
> probably nothing to do here such as reverting this change.
> 
> However, Sheng, we should treat this as an outage.  There are a couple
problems
> I see here:
> 
> - There is no BUG id for this CL, please ensure future CLs you post reference
a
> filed bug
> 
> - There was no TESTS line which suggests you did not test this patch before
> landing it.
> 
> - If there were no manual tests, it's possible there were automated tests, but
I
> presume that we don't have automated tests to catch this.  What automated
tests
> are we missing that would have verified the codereview.settings file is valid
so
> in the future someone won't manually change this line again and cause the same
> error?
> 
> Can you write a short postmortem that captures the essential parts and
includes
> some practical ideas that, if implemented, would have caught this prior to
> landing in the repo (preferably in a testing phase of writing the commit).

Hi Chase,

There's a linked CL(https://codereview.chromium.org/287063002/) which does the
real 
code and unit test changes of git_cl. However, the unit test in
process_issue_test
was not updated which would otherwise catch the issue during testing.

I'll write a postmortem about it.