Reuse gcl.py's code review settings when running [gcl try], as gcl is ...

Reuse gcl.py's code review settings when running [gcl try], as gcl is 
smarter about finding the most appropriate settings file.

Before this change, I inadvertently did a [gcl try] on a chrome-internal
change (it was a trivial change and not really secret, so no harm done).
After this change, that won't be possible.

Also, change breakpad to not upload the exception that is thrown when 
the user fails to provide correct log-on credentials.

TEST=none
BUG=none



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61738

[Jói, 2010-09-17 21:55:16]

Did you want me to change the approach based on what you mentioned at
lunch before you take a look?  I think you said something about
preferring to import gcl and use that; I guess we'd still need a flag
to tell when to import that?  Or do you want to use the codereview
settings code from gcl any time we can successfully import gcl?

Cheers,
Jói



On Fri, Sep 17, 2010 at 12:11 PM,  <joi@chromium.org> wrote:
> Reviewers: Marc-Antoine Ruel,
>
> Description:
> Reuse gcl.py's code review settings when running [gcl try], as gcl is
> smarter about finding the most appropriate settings file.
>
> Before this change, I inadvertently did a [gcl try] on a chrome-internal
> change (it was a trivial change and not really secret, so no harm done).
> After this change, that won't be possible.
>
> Also, change breakpad to not upload the exception that is thrown when
> the user fails to provide correct log-on credentials.
>
> TEST=none
> BUG=none
>
>
>
> Please review this at http://codereview.chromium.org/3390013/show
>
> SVN Base: svn://chrome-svn/chrome/trunk/tools/depot_tools/
>
> Affected files:
>  M     breakpad.py
>  M     gcl.py
>  M     tests/trychange_unittest.py
>  M     trychange.py
>
>
>

[M-A Ruel, 2010-09-17 22:29:02]

http://codereview.chromium.org/3390013/diff/3001/4001
File breakpad.py (right):

http://codereview.chromium.org/3390013/diff/3001/4001#newcode60
breakpad.py:60: isinstance(last_value, third_party.upload.ClientLoginError)):
It's be saner to just catch it in git-cl and gcl.py.

http://codereview.chromium.org/3390013/diff/3001/4002
File gcl.py (right):

http://codereview.chromium.org/3390013/diff/3001/4002#newcode1334
gcl.py:1334: except gclient_utils.Error, e:
Add the catch here.

http://codereview.chromium.org/3390013/diff/3001/4004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/3001/4004#newcode154
trychange.py:154: def ImportGclSettings(self, gcl_settings):
I'd prefer to try: import gcl instead.

[Jói, 2010-10-06 15:56:33]

Finally getting back to this thing after being buried alive in a flood of
issues.

http://codereview.chromium.org/3390013/diff/3001/4001
File breakpad.py (right):

http://codereview.chromium.org/3390013/diff/3001/4001#newcode60
breakpad.py:60: isinstance(last_value, third_party.upload.ClientLoginError)):
On 2010/09/17 22:29:02, Marc-Antoine Ruel wrote:
> It's be saner to just catch it in git-cl and gcl.py.

Done.

http://codereview.chromium.org/3390013/diff/3001/4002
File gcl.py (right):

http://codereview.chromium.org/3390013/diff/3001/4002#newcode1334
gcl.py:1334: except gclient_utils.Error, e:
On 2010/09/17 22:29:02, Marc-Antoine Ruel wrote:
> Add the catch here.

Done, here and similar place in git-try

http://codereview.chromium.org/3390013/diff/3001/4004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/3001/4004#newcode154
trychange.py:154: def ImportGclSettings(self, gcl_settings):
On 2010/09/17 22:29:02, Marc-Antoine Ruel wrote:
> I'd prefer to try: import gcl instead.

Do you mean you don't want the added codereview_settings parameter on the
TryChange function, and instead want me to try to import gcl instead?  I think
that will always succeed, so should I remove the other codereview settings code
from trychange.py ?

[M-A Ruel, 2010-10-06 16:42:21]

I'm fine with the design, just a few nits.

http://codereview.chromium.org/3390013/diff/10001/11001
File gcl.py (right):

http://codereview.chromium.org/3390013/diff/10001/11001#newcode873
gcl.py:873: del CODEREVIEW_SETTINGS['__just_initialized']
Why?

http://codereview.chromium.org/3390013/diff/10001/11002
File git-try (right):

http://codereview.chromium.org/3390013/diff/10001/11002#newcode63
git-try:63: except third_party.upload.ClientLoginError, e:
Shouldn't this be added to gcl.py too? That'd be a sane idea.

http://codereview.chromium.org/3390013/diff/10001/11002#newcode64
git-try:64: print('Got an exception')
'Got an exception while trying to login to Rietveld'

http://codereview.chromium.org/3390013/diff/10001/11004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/10001/11004#newcode163
trychange.py:163: 'port': ImportValue('TRYSERVER_HTTP_PORT'),
what you want is this form:
'port': gcl_settings.get('TRYSERVER_HTTP_PORT', None),

no need for a custom function for that.

http://codereview.chromium.org/3390013/diff/10001/11004#newcode484
trychange.py:484: codereview_settings: A codereview settings object from gcl.py
Dict containing settings

it's not much an object.

[Jói, 2010-10-06 18:52:23]

Here's a new version. I've tested manually using gcl and the unit tests pass,
but not sure how to test with git-try; the code path is now the very same for
both invocations of trychange.py though.

http://codereview.chromium.org/3390013/diff/10001/11001
File gcl.py (right):

http://codereview.chromium.org/3390013/diff/10001/11001#newcode873
gcl.py:873: del CODEREVIEW_SETTINGS['__just_initialized']
On 2010/10/06 16:42:21, Marc-Antoine Ruel wrote:
> Why?

No idea, was copying code from elsewhere in the file.

http://codereview.chromium.org/3390013/diff/10001/11002
File git-try (right):

http://codereview.chromium.org/3390013/diff/10001/11002#newcode63
git-try:63: except third_party.upload.ClientLoginError, e:
On 2010/10/06 16:42:21, Marc-Antoine Ruel wrote:
> Shouldn't this be added to gcl.py too? That'd be a sane idea.

Sorry, thought I had already.

http://codereview.chromium.org/3390013/diff/10001/11002#newcode64
git-try:64: print('Got an exception')
On 2010/10/06 16:42:21, Marc-Antoine Ruel wrote:
> 'Got an exception while trying to login to Rietveld'

Done.

http://codereview.chromium.org/3390013/diff/10001/11004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/10001/11004#newcode163
trychange.py:163: 'port': ImportValue('TRYSERVER_HTTP_PORT'),
On 2010/10/06 16:42:21, Marc-Antoine Ruel wrote:
> what you want is this form:
> 'port': gcl_settings.get('TRYSERVER_HTTP_PORT', None),
> 
> no need for a custom function for that.

This whole function got removed.

http://codereview.chromium.org/3390013/diff/10001/11004#newcode484
trychange.py:484: codereview_settings: A codereview settings object from gcl.py
On 2010/10/06 16:42:21, Marc-Antoine Ruel wrote:
> Dict containing settings
> 
> it's not much an object.

This got removed.

[Jói, 2010-10-06 18:59:40]

I found a rudimentary way to test my changes using git-try as well, seems to
work fine.

[Jói, 2010-10-06 22:09:23]

ping

On Wed, Oct 6, 2010 at 2:59 PM,  <joi@chromium.org> wrote:
> I found a rudimentary way to test my changes using git-try as well, seems to
> work fine.
>
> http://codereview.chromium.org/3390013/show
>

[M-A Ruel, 2010-10-06 23:43:18]

lgtm with nits

http://codereview.chromium.org/3390013/diff/14002/18004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/14002/18004#newcode37
trychange.py:37: GclGetCodeReviewSetting = None
I'd prefer gcl = None and use if gcl: return gcl.GetCodeReviewSetting(key)
around line 119.

http://codereview.chromium.org/3390013/diff/14002/18004#newcode41
trychange.py:41: except:
except ImportError:

[Jói, 2010-10-06 23:54:03]

Thanks. Nits fixed and committed as discussed.

http://codereview.chromium.org/3390013/diff/14002/18004
File trychange.py (right):

http://codereview.chromium.org/3390013/diff/14002/18004#newcode37
trychange.py:37: GclGetCodeReviewSetting = None
On 2010/10/06 23:43:18, Marc-Antoine Ruel wrote:
> I'd prefer gcl = None and use if gcl: return gcl.GetCodeReviewSetting(key)
> around line 119.

Done.

http://codereview.chromium.org/3390013/diff/14002/18004#newcode41
trychange.py:41: except:
On 2010/10/06 23:43:18, Marc-Antoine Ruel wrote:
> except ImportError:

Done.