add urlSchemaVersion to rebaseline_server link on buildbot console page

add urlSchemaVersion to rebaseline_server link on buildbot console page

I added a "urlSchemaVersion" parameter to the rebaseline_server URL in https://codereview.chromium.org/368933002/ ('rebaseline_server: add urlSchemaVersion parameter').

For now, the version number is 0 anyway, so users don't get a warning when they click on the link on the console page.  But once that version number is increased, users will get a warning message.  Until this fix goes in.

[epoger, 2014-07-02 21:15:12]

First LG wins.

https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html
File master/templates/layout.html (right):

https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html...
master/templates/layout.html:684: <div style="float:right; width: 195px;"><ul
style="margin: 0px 0px 0px 25px; padding:0;"><li><a
href="http://skia-tree-status.appspot.com/redirect/rebaseline-server/static/view.html#/view.html?urlSchemaVersion=current&resultsToLoad=/results/failures"
target="_blank">Recent GM Failures</a><br/></li></ul></div>
The special-case urlSchemaVersion "current" is appropriate in this case,
because:

1. This URL is relatively simple; it doesn't specify any nondefault filter
parameters, etc., which are the things that will change when the
urlSchemaVersion changes.

2. We want to be able to increment the urlSchemaVersion without changing this
file and doing a master restart.

[borenet, 2014-07-02 21:24:35]

https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html
File master/templates/layout.html (right):

https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html...
master/templates/layout.html:684: <div style="float:right; width: 195px;"><ul
style="margin: 0px 0px 0px 25px; padding:0;"><li><a
href="http://skia-tree-status.appspot.com/redirect/rebaseline-server/static/view.html#/view.html?urlSchemaVersion=current&resultsToLoad=/results/failures"
target="_blank">Recent GM Failures</a><br/></li></ul></div>
On 2014/07/02 21:15:11, epoger wrote:
> The special-case urlSchemaVersion "current" is appropriate in this case,
> because:
> 
> 1. This URL is relatively simple; it doesn't specify any nondefault filter
> parameters, etc., which are the things that will change when the
> urlSchemaVersion changes.
> 
> 2. We want to be able to increment the urlSchemaVersion without changing this
> file and doing a master restart.

Why isn't "current" the default?

[epoger, 2014-07-02 21:29:16]

On 2014/07/02 21:24:35, borenet wrote:
> https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html
> File master/templates/layout.html (right):
> 
>
https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html...
> master/templates/layout.html:684: <div style="float:right; width: 195px;"><ul
> style="margin: 0px 0px 0px 25px; padding:0;"><li><a
>
href="http://skia-tree-status.appspot.com/redirect/rebaseline-server/static/view.html#/view.html?urlSchemaVersion=current&resultsToLoad=/results/failures"
> target="_blank">Recent GM Failures</a><br/></li></ul></div>
> On 2014/07/02 21:15:11, epoger wrote:
> > The special-case urlSchemaVersion "current" is appropriate in this case,
> > because:
> > 
> > 1. This URL is relatively simple; it doesn't specify any nondefault filter
> > parameters, etc., which are the things that will change when the
> > urlSchemaVersion changes.
> > 
> > 2. We want to be able to increment the urlSchemaVersion without changing
this
> > file and doing a master restart.
> 
> Why isn't "current" the default?

Good question.  It would have been if I had been smart enough to include it from
the very beginning.  But now people may have rebaseline_server URLs (recorded in
bugs, etc.) that do not specify a urlSchemaVersion, and if I want to keep
handling those right I need to assume that no urlSchemaVersion means
urlSchemaVersion=0.

I could make "current" the default, and allow any URLs people have stored to
date to break.

Which of those is better, do you think?  (Or maybe I'm missing a clever third
way?)

[borenet, 2014-07-02 22:24:36]

On 2014/07/02 21:29:16, epoger wrote:
> On 2014/07/02 21:24:35, borenet wrote:
> >
https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html
> > File master/templates/layout.html (right):
> > 
> >
>
https://codereview.chromium.org/367173002/diff/1/master/templates/layout.html...
> > master/templates/layout.html:684: <div style="float:right; width:
195px;"><ul
> > style="margin: 0px 0px 0px 25px; padding:0;"><li><a
> >
>
href="http://skia-tree-status.appspot.com/redirect/rebaseline-server/static/view.html#/view.html?urlSchemaVersion=current&resultsToLoad=/results/failures"
> > target="_blank">Recent GM Failures</a><br/></li></ul></div>
> > On 2014/07/02 21:15:11, epoger wrote:
> > > The special-case urlSchemaVersion "current" is appropriate in this case,
> > > because:
> > > 
> > > 1. This URL is relatively simple; it doesn't specify any nondefault filter
> > > parameters, etc., which are the things that will change when the
> > > urlSchemaVersion changes.
> > > 
> > > 2. We want to be able to increment the urlSchemaVersion without changing
> this
> > > file and doing a master restart.
> > 
> > Why isn't "current" the default?
> 
> Good question.  It would have been if I had been smart enough to include it
from
> the very beginning.  But now people may have rebaseline_server URLs (recorded
in
> bugs, etc.) that do not specify a urlSchemaVersion, and if I want to keep
> handling those right I need to assume that no urlSchemaVersion means
> urlSchemaVersion=0.
> 
> I could make "current" the default, and allow any URLs people have stored to
> date to break.
> 
> Which of those is better, do you think?  (Or maybe I'm missing a clever third
> way?)

Yeah, I guess that makes sense.  It just seems like a lot of added complexity. 
It makes me wonder whether it's worth doing this over just updating the links in
the issue tracker.  I can't imagine there are that many already.  The code LGTM
at any rate.

[epoger, 2014-07-02 22:27:57]

On 2014/07/02 22:24:36, borenet wrote:
> Yeah, I guess that makes sense.  It just seems like a lot of added complexity.

> It makes me wonder whether it's worth doing this over just updating the links
in
> the issue tracker.  I can't imagine there are that many already.  The code
LGTM
> at any rate.

I really do think it's a judgment call that could go either way, and I would
actually like to defer to you on it.  Shall I make "current" the default, and
allow those old links to break?  (That would eliminate the need for this CL at
all; I would have to make a simplifying change to the rebaseline_server code
instead.)

[borenet, 2014-07-02 22:44:59]

On 2014/07/02 22:27:57, epoger wrote:
> On 2014/07/02 22:24:36, borenet wrote:
> > Yeah, I guess that makes sense.  It just seems like a lot of added
complexity.
> 
> > It makes me wonder whether it's worth doing this over just updating the
links
> in
> > the issue tracker.  I can't imagine there are that many already.  The code
> LGTM
> > at any rate.
> 
> I really do think it's a judgment call that could go either way, and I would
> actually like to defer to you on it.  Shall I make "current" the default, and
> allow those old links to break?  (That would eliminate the need for this CL at
> all; I would have to make a simplifying change to the rebaseline_server code
> instead.)

Data point: I searched the issue tracker for
"http://skia-tree-status.appspot.com/redirect/rebaseline-server" and found this
list of 6 bugs, of which one is open and doesn't actually link to a specific set
of results:
https://code.google.com/p/skia/issues/list?can=1&q=http%3A%2F%2Fskia-tree-sta...

It's possible that there are other URLs in play (maybe use of a particular IP
address), but if the cost of doing this means that we have to provide
"urlSchemaVersion=current" forever after, then my opinion is that we should just
update the existing links to use the right parameters (or just let them break). 
If it turns out that we have a large number of outdated links, then this is a
fine approach.

[epoger, 2014-07-03 17:52:51]

On 2014/07/02 22:44:59, borenet wrote:
> On 2014/07/02 22:27:57, epoger wrote:
> > On 2014/07/02 22:24:36, borenet wrote:
> > > Yeah, I guess that makes sense.  It just seems like a lot of added
> complexity.
> > 
> > > It makes me wonder whether it's worth doing this over just updating the
> links
> > in
> > > the issue tracker.  I can't imagine there are that many already.  The code
> > LGTM
> > > at any rate.
> > 
> > I really do think it's a judgment call that could go either way, and I would
> > actually like to defer to you on it.  Shall I make "current" the default,
and
> > allow those old links to break?  (That would eliminate the need for this CL
at
> > all; I would have to make a simplifying change to the rebaseline_server code
> > instead.)
> 
> Data point: I searched the issue tracker for
> "http://skia-tree-status.appspot.com/redirect/rebaseline-server" and found
this
> list of 6 bugs, of which one is open and doesn't actually link to a specific
set
> of results:
>
https://code.google.com/p/skia/issues/list?can=1&q=http%3A%2F%2Fskia-tree-sta...
> 
> It's possible that there are other URLs in play (maybe use of a particular IP
> address), but if the cost of doing this means that we have to provide
> "urlSchemaVersion=current" forever after, then my opinion is that we should
just
> update the existing links to use the right parameters (or just let them
break). 
> If it turns out that we have a large number of outdated links, then this is a
> fine approach.

Committed https://codereview.chromium.org/364253003/ ('rebaseline_server: if
urlSchemaVersion is not specified, assume current') , so this is no longer
needed.  Thanks for the help shaping this, Eric.