GTTF: Add ReadOnlyRietveld similar to one currently in CQ codebase.

GTTF: Add ReadOnlyRietveld similar to one currently in CQ codebase.

This will replace the CQ one and allow it to stay in sync with Rietveld.

For now I've only covered methods that CQ seems to touch.

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

[Isaac (away), 2013-09-14 07:48:08]

https://codereview.chromium.org/24095007/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/24095007/diff/1/rietveld.py#newcode450
rietveld.py:450: class ReadOnlyRietveld(Rietveld):
I think maybe this should not extend rietveld -- that seems like a recipe for
failure, if we accidentally use a base method which is not read only.  Perhaps
we could instantiate a private rietveld instance instead.

[Paweł Hajdan Jr., 2013-09-16 14:33:52]

https://codereview.chromium.org/24095007/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/24095007/diff/1/rietveld.py#newcode450
rietveld.py:450: class ReadOnlyRietveld(Rietveld):
On 2013/09/14 07:48:08, Isaac wrote:
> I think maybe this should not extend rietveld -- that seems like a recipe for
> failure, if we accidentally use a base method which is not read only.  Perhaps
> we could instantiate a private rietveld instance instead.

Sounds good to me - I'll probably do that on Thursday or maybe earlier if time
allows. Thanks for sharing that idea, I like it.

[M-A Ruel, 2013-09-16 14:53:04]

https://codereview.chromium.org/24095007/diff/1/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/24095007/diff/1/rietveld.py#newcode450
rietveld.py:450: class ReadOnlyRietveld(Rietveld):
On 2013/09/16 14:33:52, Paweł Hajdan Jr. wrote:
> On 2013/09/14 07:48:08, Isaac wrote:
> > I think maybe this should not extend rietveld -- that seems like a recipe
for
> > failure, if we accidentally use a base method which is not read only. 
Perhaps
> > we could instantiate a private rietveld instance instead.
> 
> Sounds good to me - I'll probably do that on Thursday or maybe earlier if time
> allows. Thanks for sharing that idea, I like it.

Technically, you could just zap POST and be fine. But I don't mind either way.

[iannucci, 2013-09-16 17:18:14]

On 2013/09/14 07:48:08, Isaac wrote:
> https://codereview.chromium.org/24095007/diff/1/rietveld.py
> File rietveld.py (right):
> 
> https://codereview.chromium.org/24095007/diff/1/rietveld.py#newcode450
> rietveld.py:450: class ReadOnlyRietveld(Rietveld):
> I think maybe this should not extend rietveld -- that seems like a recipe for
> failure, if we accidentally use a base method which is not read only.  Perhaps
> we could instantiate a private rietveld instance instead.

I think running a private rietveld instance is a valid approach sometimes. There
could be a PrivateRietveld class to do this. I think that as much as possible,
ReadOnlyRietveld should really be a read-only connection to rietveld.

Zapping POST requests as M-A mentions would probably work fine, and then
implementing a copy-on-write layer for apis which are meant to be persistent
(i.e. setting flags on a CL).

[Paweł Hajdan Jr., 2013-09-19 22:02:39]

PTAL

[M-A Ruel, 2013-09-19 22:20:27]

lgtm

https://codereview.chromium.org/24095007/diff/8001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/24095007/diff/8001/rietveld.py#newcode451
rietveld.py:451: # logic to be invoked accidentally.
put that in the docstring

[Paweł Hajdan Jr., 2013-09-20 19:30:29]

https://codereview.chromium.org/24095007/diff/8001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/24095007/diff/8001/rietveld.py#newcode451
rietveld.py:451: # logic to be invoked accidentally.
On 2013/09/19 22:20:27, M-A Ruel wrote:
> put that in the docstring

Done.

[commit-bot: I haz the power, 2013-09-20 19:30:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/24095007/13001

[commit-bot: I haz the power, 2013-09-20 19:33:00]

Presubmit check for 24095007-13001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
tests/gclient_smoketest.py failed
.......F.................................
======================================================================
FAIL: testMultiSolutionsJobs (__main__.GClientSmokeBoth)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1151, in testMultiSolutionsJobs
    results = self.splitBlock(stdout)
  File "tests/gclient_smoketest.py", line 115, in splitBlock
    self.fail(line)
AssertionError: __.GClientSmokeBoth.testMultiSolutionsJobs/src/other/origin

----------------------------------------------------------------------
Ran 41 tests in 72.633s

FAILED (failures=1)


Presubmit checks took 132.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-09-20 19:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/24095007/13001

[commit-bot: I haz the power, 2013-09-20 19:38:49]

Change committed as 224455

[Isaac (away), 2013-09-20 19:39:17]

On 2013/09/16 17:18:14, iannucci wrote:
> On 2013/09/14 07:48:08, Isaac wrote:
> > https://codereview.chromium.org/24095007/diff/1/rietveld.py
> > File rietveld.py (right):
> > 
> > https://codereview.chromium.org/24095007/diff/1/rietveld.py#newcode450
> > rietveld.py:450: class ReadOnlyRietveld(Rietveld):
> > I think maybe this should not extend rietveld -- that seems like a recipe
for
> > failure, if we accidentally use a base method which is not read only. 
Perhaps
> > we could instantiate a private rietveld instance instead.
> 
> I think running a private rietveld instance is a valid approach sometimes.
There
> could be a PrivateRietveld class to do this. I think that as much as possible,
> ReadOnlyRietveld should really be a read-only connection to rietveld.
> 
> Zapping POST requests as M-A mentions would probably work fine, and then
> implementing a copy-on-write layer for apis which are meant to be persistent
> (i.e. setting flags on a CL).

Sorry for being unclear. I meant ReadOnlyRietveld could have a instance variable
pointing to an instantiation of the Rietveld class, instead of subclassing it.
I didn't mean we needed to launch Rietveld on dev_appserver.

Looks like everything got sorted out in the end.