LogDog: Update client/bootstrap to generate URLs.

LogDog: Update client/bootstrap to generate URLs.

Add the ability for a populated Bootstrap to generate viewer URLs.

Update streamclient.Stream to be able to emit its stream properties. This is
useful in general, notably by the Bootstrap's URL generation functions.

This change involves modifying streamproto.Properties to remove the false
assumption that they are safe to copy/modify. To this end, places where
this assumption was made now properly Clone() the Properties to take
possession of them, and Properties are now passed around as a pointer to
make this clear.

BUG=chromium:659291
TEST=unit

Committed: https://github.com/luci/luci-go/commit/16e6635d9d47ce93ee0bc0cd952e53abbd95a686

[dnj, 2016-10-27 23:39:37]

Patchset #1 (id:1) has been deleted

[dnj, 2016-10-27 23:39:50]

dnj@chromium.org changed reviewers:
+ nodir@chromium.org, vadimsh@chromium.org

[dnj, 2016-10-27 23:39:51]

PTAL

[Vadim Sh., 2016-10-27 23:50:05]

lgtm with nits

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
File logdog/client/butlerlib/bootstrap/bootstrap.go (right):

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
logdog/client/butlerlib/bootstrap/bootstrap.go:90: // GetViewerURL returns a URL
to the
to the what? :)

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
logdog/client/butlerlib/bootstrap/bootstrap.go:92: // This requires the
Bootstrap to have access to the Coordinator host
by "access to the Coordinator host" you mean the configure envionment variable
when Bootstrap was created?

https://codereview.chromium.org/2456953003/diff/20001/logdog/common/viewer/ur...
File logdog/common/viewer/url.go (right):

https://codereview.chromium.org/2456953003/diff/20001/logdog/common/viewer/ur...
logdog/common/viewer/url.go:20: pathQueries := make([]string, len(paths))
nit: use https://golang.org/pkg/net/url/#Values.Add and
https://golang.org/pkg/net/url/#Values.Encode

[dnj, 2016-10-28 00:01:45]

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
File logdog/client/butlerlib/bootstrap/bootstrap.go (right):

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
logdog/client/butlerlib/bootstrap/bootstrap.go:90: // GetViewerURL returns a URL
to the
On 2016/10/27 23:50:05, Vadim Sh. wrote:
> to the what? :)

Done.

https://codereview.chromium.org/2456953003/diff/20001/logdog/client/butlerlib...
logdog/client/butlerlib/bootstrap/bootstrap.go:92: // This requires the
Bootstrap to have access to the Coordinator host
On 2016/10/27 23:50:05, Vadim Sh. wrote:
> by "access to the Coordinator host" you mean the configure envionment variable
> when Bootstrap was created?

Or populated, yeah. Clarified in comment.

https://codereview.chromium.org/2456953003/diff/20001/logdog/common/viewer/ur...
File logdog/common/viewer/url.go (right):

https://codereview.chromium.org/2456953003/diff/20001/logdog/common/viewer/ur...
logdog/common/viewer/url.go:20: pathQueries := make([]string, len(paths))
On 2016/10/27 23:50:05, Vadim Sh. wrote:
> nit: use https://golang.org/pkg/net/url/#Values.Add and
> https://golang.org/pkg/net/url/#Values.Encode

Oh shoot, I can't believe I forgot about this. Done, using "net/url.URL" for
everything.

[dnj, 2016-10-28 00:01:52]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 00:01:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 01:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 01:04:09]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3221698c98718810)

[dnj, 2016-10-28 03:55:47]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-10-28 03:55:48]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2456953003/#ps40001
(title: "Better comments, use "url.URL".")

[commit-bot: I haz the power, 2016-10-28 03:55:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Vadim Sh., 2016-10-28 03:56:53]

https://codereview.chromium.org/2456953003/diff/40001/logdog/common/viewer/ur...
File logdog/common/viewer/url.go (right):

https://codereview.chromium.org/2456953003/diff/40001/logdog/common/viewer/ur...
logdog/common/viewer/url.go:19: query := make(url.Values, len(paths))
nit: len(paths) here is harmful.

url.Values is map[string][]string, it allocated capacity for the wrong thing

[dnj, 2016-10-28 04:00:10]

https://codereview.chromium.org/2456953003/diff/40001/logdog/common/viewer/ur...
File logdog/common/viewer/url.go (right):

https://codereview.chromium.org/2456953003/diff/40001/logdog/common/viewer/ur...
logdog/common/viewer/url.go:19: query := make(url.Values, len(paths))
On 2016/10/28 03:56:53, Vadim Sh. wrote:
> nit: len(paths) here is harmful.
> 
> url.Values is map[string][]string, it allocated capacity for the wrong thing

Hah you're right, didn't occur to me that every key was "s". Good catch.

[dnj, 2016-10-28 04:00:14]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2016-10-28 04:04:31]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-10-28 04:04:32]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2456953003/#ps60001
(title: "Not stupid allocation.")

[commit-bot: I haz the power, 2016-10-28 04:04:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnj, 2016-10-28 04:16:27]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-10-28 04:16:27]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2456953003/#ps80001
(title: "Winders")

[commit-bot: I haz the power, 2016-10-28 04:16:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 05:18:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 05:18:41]

Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3222529039cb8510)

[dnj, 2016-10-28 06:57:43]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-10-28 06:57:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 07:04:23]

Description was changed from

==========
LogDog: Update client/bootstrap to generate URLs.

Add the ability for a populated Bootstrap to generate viewer URLs.

Update streamclient.Stream to be able to emit its stream properties. This is
useful in general, notably by the Bootstrap's URL generation functions.

This change involves modifying streamproto.Properties to remove the false
assumption that they are safe to copy/modify. To this end, places where
this assumption was made now properly Clone() the Properties to take
possession of them, and Properties are now passed around as a pointer to
make this clear.

BUG=chromium:659291
TEST=unit
==========

to

==========
LogDog: Update client/bootstrap to generate URLs.

Add the ability for a populated Bootstrap to generate viewer URLs.

Update streamclient.Stream to be able to emit its stream properties. This is
useful in general, notably by the Bootstrap's URL generation functions.

This change involves modifying streamproto.Properties to remove the false
assumption that they are safe to copy/modify. To this end, places where
this assumption was made now properly Clone() the Properties to take
possession of them, and Properties are now passed around as a pointer to
make this clear.

BUG=chromium:659291
TEST=unit

Committed:
https://github.com/luci/luci-go/commit/16e6635d9d47ce93ee0bc0cd952e53abbd95a686
==========

[commit-bot: I haz the power, 2016-10-28 07:04:24]

Committed patchset #4 (id:80001) as
https://github.com/luci/luci-go/commit/16e6635d9d47ce93ee0bc0cd952e53abbd95a686