milo: Add SwarmBucket templating.

milo: Add SwarmBucket templating.

Add SwarmBucket template resolution package. Employ this in Milo when
resolving the "log_location" URL. This will enable SwarmBucket templates
that include a Swarming run ID in their name to resolve to their actual
annotation log stream.

BUG=chromium:692217
TEST=unit

Review-Url: https://codereview.chromium.org/2701033003
Committed: https://github.com/luci/luci-go/commit/fc6bd8a8856796312b4a2e3c043a74dac66ac194

[dnj, 2017-02-17 21:46:59]

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

[dnj, 2017-02-17 21:46:59]

PTAL

[hinoka, 2017-02-17 22:07:40]

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
File buildbucket/swarmbucket/subst/subst.go (right):

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:5: // Package subst is a generic string
template resolver.
subst is confusing, how about just "substitution" or "stringsub"?

It's also not generic, it's pretty specific for our usage and looks for very
specific strings in the template.  It resolves string templates used
specifically in swarmbucket into logdog urls.

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:29: var namedFormatMatcher =
regexp.MustCompile(`(?:(^|[^\\])\$)\{([^\}]+)(\}|$)`)
What about using https://golang.org/pkg/text/template/ instead of regex?

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:44: // Resolve performs string
resolution.
Of what to what?

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:49: func (s *Subst) Resolve(v string)
(string, error) {
If we have less than 3 inputs, i think a function is a simpler interface (ie You
ain't gonna need it).

func Resolve(SwarmingRunID, Template string) (string, error) {

[nodir, 2017-02-18 03:19:48]

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/templatestr/template.go (right):

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:1: // Copyright 2017 The LUCI
Authors. All rights reserved.
the template format is not buildbucket specific really, swarmbucket uses python
string.Template for that.
Therefore I don't think it should be in buildbucket package, maybe in common?

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:29: var namedFormatMatcher =
regexp.MustCompile(`(?:(^|[^\\])\$)\{([^\}]+)(\}|$)`)
buildbucket uses python string.Template class for this. Let's make this package
compatible with string.Template.
Please use the regexp in
https://github.com/python/cpython/blob/master/Lib/string.py#L55
see identity regexp too

note that they use $$, not \$, for escaping

[nodir, 2017-02-18 04:27:52]

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/templatestr/template.go (right):

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:41: SwarmingRunID string
i think this package mixes code of different abstraction levels. On one hand,
there is a very generic implementation of parameter substitution. On the other
hand, there is swarmbucket (swarming really) specific parameter name. If this
package is about swarmbucket, it could use just string.Replace.

[dnj, 2017-02-18 04:48:32]

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
File buildbucket/swarmbucket/subst/subst.go (right):

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:5: // Package subst is a generic string
template resolver.
On 2017/02/17 22:07:40, hinoka wrote:
> subst is confusing, how about just "substitution" or "stringsub"?
> 
> It's also not generic, it's pretty specific for our usage and looks for very
> specific strings in the template.  It resolves string templates used
> specifically in swarmbucket into logdog urls.

Changed to "template_string". This isn't just for LogDog; it's for potentially
any SwarmBucket substitution.

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:29: var namedFormatMatcher =
regexp.MustCompile(`(?:(^|[^\\])\$)\{([^\}]+)(\}|$)`)
On 2017/02/17 22:07:40, hinoka wrote:
> What about using https://golang.org/pkg/text/template/ instead of regex?

template is more of a document-wide package. It's also way more powerful than I
want here.

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:44: // Resolve performs string
resolution.
On 2017/02/17 22:07:40, hinoka wrote:
> Of what to what?

Done.

https://codereview.chromium.org/2701033003/diff/1/buildbucket/swarmbucket/sub...
buildbucket/swarmbucket/subst/subst.go:49: func (s *Subst) Resolve(v string)
(string, error) {
On 2017/02/17 22:07:40, hinoka wrote:
> If we have less than 3 inputs, i think a function is a simpler interface (ie
You
> ain't gonna need it).
> 
> func Resolve(SwarmingRunID, Template string) (string, error) {

I think a struct is good, (a) because if we add more then it'll be super easy,
and (b) b/c "string, string" functions allow for the easy possibility of
accidentally swapping parameter orders and messing everything up.

[dnj, 2017-02-18 06:08:13]

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/templatestr/template.go (right):

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:1: // Copyright 2017 The LUCI
Authors. All rights reserved.
On 2017/02/18 03:19:48, nodir wrote:
> the template format is not buildbucket specific really, swarmbucket uses
python
> string.Template for that.
> Therefore I don't think it should be in buildbucket package, maybe in common?

Acknowledged.

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:29: var namedFormatMatcher =
regexp.MustCompile(`(?:(^|[^\\])\$)\{([^\}]+)(\}|$)`)
On 2017/02/18 03:19:48, nodir wrote:
> buildbucket uses python string.Template class for this. Let's make this
package
> compatible with string.Template.
> Please use the regexp in
> https://github.com/python/cpython/blob/master/Lib/string.py#L55
> see identity regexp too
> 
> note that they use $$, not \$, for escaping

I went ahead and implemented a Python-style string.Template package.

https://codereview.chromium.org/2701033003/diff/20001/buildbucket/swarmbucket...
buildbucket/swarmbucket/templatestr/template.go:41: SwarmingRunID string
On 2017/02/18 04:27:52, nodir wrote:
> i think this package mixes code of different abstraction levels. On one hand,
> there is a very generic implementation of parameter substitution. On the other
> hand, there is swarmbucket (swarming really) specific parameter name. If this
> package is about swarmbucket, it could use just string.Replace. 

I was leaving room for future substitutions.

[nodir, 2017-02-18 07:00:55]

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/tasktemplate/template.go (right):

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
buildbucket/swarmbucket/tasktemplate/template.go:5: // Package tasktemplate
resolves Swarming task templates.
a swarming task template is a JSON object that is turned into a swarming task
request. This package does not deal with that. Rather, it deals specifically
with "log_location" swarming tag.

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
buildbucket/swarmbucket/tasktemplate/template.go:19: type Params struct {
i think DM would use the same log_location swarming tag with the same value
format to specify where Milo should find the annotations. Therefore this is not
really swarmbucket specific, but rather swarming. If we have other tags,
unrelated to logging, but that has to have the task id, we'd probably use this
code again.

Even having said that, I am not sure there is a well-defined abstraction level
for this, therefore I am not sure this deserves its own package or whether it
should be extracted out of Milo. I'd move this code to milo/appengine/swarming/,
and, honestly, make it a simple func(template, runid string) to keep things
simple and not overgeneralized. Most of the value of this package was the
parameter substitution which is now moved to a better place.

https://codereview.chromium.org/2701033003/diff/40001/common/data/text/string...
File common/data/text/stringtemplate/template.go (right):

https://codereview.chromium.org/2701033003/diff/40001/common/data/text/string...
common/data/text/stringtemplate/template.go:5: // Package stringtemplate
implements Python string.Template-like substitution.
this is a nice package, thanks

https://codereview.chromium.org/2701033003/diff/40001/common/data/text/string...
common/data/text/stringtemplate/template.go:79: }
nit: can you add `panic("impossible") in the default case, to have a peace of
mind for future modifications of this code

[dnj, 2017-02-18 07:42:48]

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/tasktemplate/template.go (right):

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
buildbucket/swarmbucket/tasktemplate/template.go:19: type Params struct {
On 2017/02/18 07:00:54, nodir wrote:
> i think DM would use the same log_location swarming tag with the same value
> format to specify where Milo should find the annotations. Therefore this is
not
> really swarmbucket specific, but rather swarming. If we have other tags,
> unrelated to logging, but that has to have the task id, we'd probably use this
> code again.
> 
> Even having said that, I am not sure there is a well-defined abstraction level
> for this, therefore I am not sure this deserves its own package or whether it
> should be extracted out of Milo. I'd move this code to
milo/appengine/swarming/,
> and, honestly, make it a simple func(template, runid string) to keep things
> simple and not overgeneralized. Most of the value of this package was the
> parameter substitution which is now moved to a better place.

Well, I want Kitchen to import this too, since it needs to do the same
substitution with the "logdog-annotation-url" flag.

https://codereview.chromium.org/2701033003/diff/40001/common/data/text/string...
File common/data/text/stringtemplate/template.go (right):

https://codereview.chromium.org/2701033003/diff/40001/common/data/text/string...
common/data/text/stringtemplate/template.go:79: }
On 2017/02/18 07:00:54, nodir wrote:
> nit: can you add `panic("impossible") in the default case, to have a peace of
> mind for future modifications of this code

Done.

[nodir, 2017-02-18 15:40:32]

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
File buildbucket/swarmbucket/tasktemplate/template.go (right):

https://codereview.chromium.org/2701033003/diff/40001/buildbucket/swarmbucket...
buildbucket/swarmbucket/tasktemplate/template.go:19: type Params struct {
On 2017/02/18 07:42:48, dnj wrote:
> On 2017/02/18 07:00:54, nodir wrote:
> > i think DM would use the same log_location swarming tag with the same value
> > format to specify where Milo should find the annotations. Therefore this is
> not
> > really swarmbucket specific, but rather swarming. If we have other tags,
> > unrelated to logging, but that has to have the task id, we'd probably use
this
> > code again.
> > 
> > Even having said that, I am not sure there is a well-defined abstraction
level
> > for this, therefore I am not sure this deserves its own package or whether
it
> > should be extracted out of Milo. I'd move this code to
> milo/appengine/swarming/,
> > and, honestly, make it a simple func(template, runid string) to keep things
> > simple and not overgeneralized. Most of the value of this package was the
> > parameter substitution which is now moved to a better place.
> 
> Well, I want Kitchen to import this too, since it needs to do the same
> substitution with the "logdog-annotation-url" flag.

you are right, this code has to be extracted to a separate package.

still, nothing in this file is about swarmbucket, there is no single mention.
All swarmbucket parameters are resolved by swarmbucket and no other
buildbucket/swarmbucket parameters will appear in log_location tag, or any other
swarming tag. DM would use the same template format.

can we move this to //swarming?

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
File milo/appengine/swarming/buildinfo.go (right):

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
milo/appengine/swarming/buildinfo.go:221: // Gather our SwarmBucket template
parameters and perform a substitution.
it is rather swarmbucket template parameters

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
milo/appengine/swarming/buildinfo.go:230: return nil,
errors.Annotate(err).Reason("failed to resolve SwarmBucket templating in
'log_location'").Err()
here too

[dnj, 2017-02-18 18:52:31]

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
File milo/appengine/swarming/buildinfo.go (right):

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
milo/appengine/swarming/buildinfo.go:221: // Gather our SwarmBucket template
parameters and perform a substitution.
On 2017/02/18 15:40:31, nodir wrote:
> it is rather swarmbucket template parameters

yeah sorry was just coming up with a name. going with "swarming task template".

https://codereview.chromium.org/2701033003/diff/60001/milo/appengine/swarming...
milo/appengine/swarming/buildinfo.go:230: return nil,
errors.Annotate(err).Reason("failed to resolve SwarmBucket templating in
'log_location'").Err()
On 2017/02/18 15:40:32, nodir wrote:
> here too

Done.

[nodir, 2017-02-18 19:11:20]

i don't see the new swarming package

[nodir, 2017-02-18 19:12:09]

lgtm after adding it to the patchset

[dnj, 2017-02-18 19:24:42]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-18 19:24:42]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2701033003/#ps120001
(title: "Apparently "/swarming" is gitignored! Added...")

[commit-bot: I haz the power, 2017-02-18 19:24:53]

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, 2017-02-18 19:31:11]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487445882129260,
"parent_rev": "c0b973a0a2e68852c035a98b7b0a4bad5d283f6a", "commit_rev":
"fc6bd8a8856796312b4a2e3c043a74dac66ac194"}

[commit-bot: I haz the power, 2017-02-18 19:31:15]

Description was changed from

==========
milo: Add SwarmBucket templating.

Add SwarmBucket template resolution package. Employ this in Milo when
resolving the "log_location" URL. This will enable SwarmBucket templates
that include a Swarming run ID in their name to resolve to their actual
annotation log stream.

BUG=chromium:692217
TEST=unit
==========

to

==========
milo: Add SwarmBucket templating.

Add SwarmBucket template resolution package. Employ this in Milo when
resolving the "log_location" URL. This will enable SwarmBucket templates
that include a Swarming run ID in their name to resolve to their actual
annotation log stream.

BUG=chromium:692217
TEST=unit

Review-Url: https://codereview.chromium.org/2701033003
Committed:
https://github.com/luci/luci-go/commit/fc6bd8a8856796312b4a2e3c043a74dac66ac194
==========

[commit-bot: I haz the power, 2017-02-18 19:31:16]

Committed patchset #7 (id:120001) as
https://github.com/luci/luci-go/commit/fc6bd8a8856796312b4a2e3c043a74dac66ac194