Milo: Remove swarming tags from build properties

Milo: Remove swarming tags from build properties

This makes the build view look cleaner.  This moves info from the swarming tag
onto other places like the execution and sourcestamp sections.

BUG=632898, 690591
Review-Url: https://codereview.chromium.org/2669473002
Committed: https://github.com/luci/luci-go/commit/d82ae617400d678575c2a8d877fcf1c9f2c44b03

[Ryan Tseng, 2017-01-31 01:39:30]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 01:39:38]

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

[hinoka, 2017-01-31 01:40:00]

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

[hinoka, 2017-01-31 01:40:01]

ptal! Demo here:

https://1439-a2b77d3-tainted-hinoka-dot-luci-milo.appspot.com/swarming/task/3...

[commit-bot: I haz the power, 2017-01-31 01:46:07]

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

[commit-bot: I haz the power, 2017-01-31 01:46:08]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-02-07 00:55:27]

Bump!

[nodir, 2017-02-07 21:56:30]

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
File
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html
(right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html:93:

we should get rid of these blank lines (in a separate CL)

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
File
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html
(right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html:82:

it should be a separate test

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
File
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html
(right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html:76:
</table>
why this wasn't there before? were expectations somehow stale?

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
File milo/appengine/frontend/templates/buildbot/pages/build.html (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/buildbot/pages/build.html:171:
<tr><th>Name</th><th>Value</th><th>Source</th></tr>
we don't need Source anymore?

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:181: result[parts[0]] = parts[1]
are you sure tag keys are unique? add a func comment that this function assumes
that

consider logging an error if this is not the case

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:188: func addBuilderLink(c context.Context,
build *resp.MiloBuild, selfURL string, props map[string]string) {
it is not obvious what "self" means here. Task? Consider changing to taskURL

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:188: func addBuilderLink(c context.Context,
build *resp.MiloBuild, selfURL string, props map[string]string) {
s/props/tags/

in swarming land, properties mean something different

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:209: func addBanner(build *resp.MiloBuild,
props map[string]string) {
s/props/tags/

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:326: func addBuildsetInfo(build
*resp.MiloBuild, props map[string]string) {
s/props/tags/

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:329: if build.SourceStamp == nil {
ah, this is why an empty SourceStamp section appeared.
consider moving this to L337

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:334: gerritset := strings.TrimLeft(patchset,
"gerrit/")
gerritPatchset?

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:339: URL:   fmt.Sprintf("https://%s/r/%s/%s",
parts[0], parts[1], parts[2]),
never saw /r/ before. 
did you mean c?

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:346: func addRecipeLink(build *resp.MiloBuild,
props map[string]string) {
tags

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:351: subpath := "/+/"
move /+/ to the format string

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:358: subpath += "scripts/slave/recipes/"
this isn't right

we should fetch recipes.cfg
(https://chromium.googlesource.com/infra/infra/+/master/infra/config/recipes.cfg)
to find the recipe_path

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:359: }
this will not work for any other repos?

for https://luci-milo.appspot.com/swarming/task/342fe1d557939c10
URL will be https://chromium.googlesource.com/infra/infra/+/master/luci_go.py
which is invalid URL

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:380: props := tagsToMap(sr.Tags)
tags

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/exp...
File milo/appengine/swarming/expectations/build-canceled.json (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/exp...
milo/appengine/swarming/expectations/build-canceled.json:61: "Recipe": null,
yeah, this looks like another example of why "build is step" is problematic

[hinoka, 2017-02-10 22:03:41]

Description was changed from

==========
Milo: Remove swarming tags from build properties

This makes the build view look cleaner.  This moves info from the swarming tag
onto other places like the execution and sourcestamp sections.

BUG=632898
==========

to

==========
Milo: Remove swarming tags from build properties

This makes the build view look cleaner.  This moves info from the swarming tag
onto other places like the execution and sourcestamp sections.

BUG=632898, 690591
==========

[nodir, 2017-02-23 01:24:12]

ping

[hinoka, 2017-02-24 00:56:22]

I'm going to table this temporarily for a couple reasons.

1. Recipe repo and name.  We're missing a way to link to recipes since they
could reside in different parts of a repo.
2. This will break Skia, who don't use kitchen but apperently rely on Milo.

https://luci-milo.appspot.com/swarming/task/348506f01d7c2810

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
File
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html
(right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html:93:

On 2017/02/07 21:56:29, nodir wrote:
> we should get rid of these blank lines (in a separate CL)

Acknowledged.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
File
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html
(right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html:76:
</table>
On 2017/02/07 21:56:29, nodir wrote:
> why this wasn't there before? were expectations somehow stale?

Actually this is a bug... sourcestamp shouldn't appear if nothing is in there.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
File milo/appengine/frontend/templates/buildbot/pages/build.html (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
milo/appengine/frontend/templates/buildbot/pages/build.html:171:
<tr><th>Name</th><th>Value</th><th>Source</th></tr>
On 2017/02/07 21:56:29, nodir wrote:
> we don't need Source anymore?

They still indicate the source step

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:181: result[parts[0]] = parts[1]
On 2017/02/07 21:56:29, nodir wrote:
> are you sure tag keys are unique? add a func comment that this function
assumes
> that
> 
> consider logging an error if this is not the case

Tags keys are supposed to be unique.  If a second step emits a conflicting key,
it gets overridden.

I'd rather not log here because i'd have to pass in a context to an otherwise
stateless function :(

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:188: func addBuilderLink(c context.Context,
build *resp.MiloBuild, selfURL string, props map[string]string) {
On 2017/02/07 21:56:30, nodir wrote:
> s/props/tags/
> 
> in swarming land, properties mean something different

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:188: func addBuilderLink(c context.Context,
build *resp.MiloBuild, selfURL string, props map[string]string) {
On 2017/02/07 21:56:30, nodir wrote:
> it is not obvious what "self" means here. Task? Consider changing to taskURL

Actually I just removed this entirely.  No need to log this since it'll be
evident by the lack of a builder link.  Also skia doesn't have this, so we'll
just get a bunch of error spam instead.  And now it looks cleaner.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:209: func addBanner(build *resp.MiloBuild,
props map[string]string) {
On 2017/02/07 21:56:29, nodir wrote:
> s/props/tags/

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:326: func addBuildsetInfo(build
*resp.MiloBuild, props map[string]string) {
On 2017/02/07 21:56:29, nodir wrote:
> s/props/tags/

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:329: if build.SourceStamp == nil {
On 2017/02/07 21:56:29, nodir wrote:
> ah, this is why an empty SourceStamp section appeared.
> consider moving this to L337

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:334: gerritset := strings.TrimLeft(patchset,
"gerrit/")
On 2017/02/07 21:56:30, nodir wrote:
> gerritPatchset?

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:339: URL:   fmt.Sprintf("https://%s/r/%s/%s",
parts[0], parts[1], parts[2]),
On 2017/02/07 21:56:30, nodir wrote:
> never saw /r/ before. 
> did you mean c?

Oops fixed.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:346: func addRecipeLink(build *resp.MiloBuild,
props map[string]string) {
On 2017/02/07 21:56:30, nodir wrote:
> tags

Done.

[nodir, 2017-02-24 02:44:55]

On Thu, Feb 23, 2017 at 4:56 PM <hinoka@chromium.org> wrote:

> I'm going to table this temporarily for a couple reasons.
>
> 1. Recipe repo and name. We're missing a way to link to recipes since they
>

I find removing tags from build page (bug) more important than providing a
link to recipe (new feature)


> could reside in different parts of a repo.
> 2. This will break Skia, who don't use kitchen but apperently rely on Milo.
>

Did you confirm that they need tags to be displayed in Milo?


>
> https://luci-milo.appspot.com/swarming/task/348506f01d7c2810
>
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
> File
>
>
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html
> (right):
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
>
>
milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_CrWinGoma_30608.html:93:
>
> On 2017/02/07 21:56:29, nodir wrote:
> > we should get rid of these blank lines (in a separate CL)
>
> Acknowledged.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
> File
>
>
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html
> (right):
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/exp...
>
>
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-expired.html:76:
> </table>
> On 2017/02/07 21:56:29, nodir wrote:
> > why this wasn't there before? were expectations somehow stale?
>
> Actually this is a bug... sourcestamp shouldn't appear if nothing is in
> there.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
> File milo/appengine/frontend/templates/buildbot/pages/build.html
> (right):
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/frontend/tem...
> milo/appengine/frontend/templates/buildbot/pages/build.html:171:
> <tr><th>Name</th><th>Value</th><th>Source</th></tr>
> On 2017/02/07 21:56:29, nodir wrote:
> > we don't need Source anymore?
>
> They still indicate the source step
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> File milo/appengine/swarming/build.go (right):
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:181: result[parts[0]] = parts[1]
> On 2017/02/07 21:56:29, nodir wrote:
> > are you sure tag keys are unique? add a func comment that this
> function assumes
> > that
> >
> > consider logging an error if this is not the case
>
> Tags keys are supposed to be unique. If a second step emits a
> conflicting key, it gets overridden.
>
> I'd rather not log here because i'd have to pass in a context to an
> otherwise stateless function :(
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:188: func addBuilderLink(c
> context.Context, build *resp.MiloBuild, selfURL string, props
> map[string]string) {
> On 2017/02/07 21:56:30, nodir wrote:
> > s/props/tags/
> >
> > in swarming land, properties mean something different
>
> Done.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:188: func addBuilderLink(c
> context.Context, build *resp.MiloBuild, selfURL string, props
> map[string]string) {
> On 2017/02/07 21:56:30, nodir wrote:
> > it is not obvious what "self" means here. Task? Consider changing to
> taskURL
>
> Actually I just removed this entirely. No need to log this since it'll
> be evident by the lack of a builder link. Also skia doesn't have this,
> so we'll just get a bunch of error spam instead. And now it looks
> cleaner.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:209: func addBanner(build
> *resp.MiloBuild, props map[string]string) {
> On 2017/02/07 21:56:29, nodir wrote:
> > s/props/tags/
>
> Done.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:326: func addBuildsetInfo(build
> *resp.MiloBuild, props map[string]string) {
> On 2017/02/07 21:56:29, nodir wrote:
> > s/props/tags/
>
> Done.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:329: if build.SourceStamp == nil {
> On 2017/02/07 21:56:29, nodir wrote:
> > ah, this is why an empty SourceStamp section appeared.
> > consider moving this to L337
>
> Done.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:334: gerritset :=
> strings.TrimLeft(patchset, "gerrit/")
> On 2017/02/07 21:56:30, nodir wrote:
> > gerritPatchset?
>
> Done.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:339: URL:
> fmt.Sprintf("https://%s/r/%s/%s", parts[0], parts[1], parts[2]),
> On 2017/02/07 21:56:30, nodir wrote:
> > never saw /r/ before.
> > did you mean c?
>
> Oops fixed.
>
>
>
https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:346: func addRecipeLink(build
> *resp.MiloBuild, props map[string]string) {
> On 2017/02/07 21:56:30, nodir wrote:
> > tags
>
> Done.
>
> https://codereview.chromium.org/2669473002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Tseng, 2017-03-01 22:47:34]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 22:47:44]

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

[Ryan Tseng, 2017-03-01 22:51:36]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 22:51:44]

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

[hinoka, 2017-03-01 22:53:10]

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:351: subpath := "/+/"
On 2017/02/07 21:56:30, nodir wrote:
> move /+/ to the format string

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:358: subpath += "scripts/slave/recipes/"
On 2017/02/07 21:56:29, nodir wrote:
> this isn't right
> 
> we should fetch recipes.cfg
>
(https://chromium.googlesource.com/infra/infra/+/master/infra/config/recipes.cfg)
> to find the recipe_path

That won't work, removing sutpath alltogether

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:359: }
On 2017/02/07 21:56:30, nodir wrote:
> this will not work for any other repos?
> 
> for https://luci-milo.appspot.com/swarming/task/342fe1d557939c10
> URL will be https://chromium.googlesource.com/infra/infra/+/master/luci_go.py
> which is invalid URL

Acknowledged.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:380: props := tagsToMap(sr.Tags)
On 2017/02/07 21:56:30, nodir wrote:
> tags

Done.

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/exp...
File milo/appengine/swarming/expectations/build-canceled.json (right):

https://codereview.chromium.org/2669473002/diff/1/milo/appengine/swarming/exp...
milo/appengine/swarming/expectations/build-canceled.json:61: "Recipe": null,
On 2017/02/07 21:56:30, nodir wrote:
> yeah, this looks like another example of why "build is step" is problematic

In theory, a "build" could be a quest that contains subquests that run different
recipes.  Time will tell if that will converge.

[commit-bot: I haz the power, 2017-03-01 22:59:14]

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

[commit-bot: I haz the power, 2017-03-01 22:59:15]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-03-01 23:25:05]

https://codereview.chromium.org/2669473002/diff/40001/milo/api/resp/build.go
File milo/api/resp/build.go (right):

https://codereview.chromium.org/2669473002/diff/40001/milo/api/resp/build.go#...
milo/api/resp/build.go:222: Recipe *Link
add `json:,omitempty` to avoid all those `"Recipe": null` in the JSON
expectation files

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/frontend...
File
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html
(right):

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html:83:
Recipe: <a
href="https://chromium.googlesource.com/chromium/tools/build/&#43;/master/">infra/infra_continuous</a>
this should be "+", not "&#43;"

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:251: result := map[string]string{}
preallocate len(tags) entries

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:253: if t == "" {
this if statement is unnecessary: strings.SpinN("", ":", 2) == []

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:414: }
that's a lot of nesting. Consider inverting your if statements and put returns

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:417: name := tags["recipe_name"]
name is not used

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:425: subpath += "master/"
it is more correct to use HEAD, not master

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:429: URL:   fmt.Sprintf("%s/+/%s", repo,
subpath),
this works only on Gitiles. Let's not do this on non-gitiles. Please check if
the repository looks like a Gitiles URL, e.g. ends with .googlesource.com

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:435: selfURL := taskPageURL(server, sr.TaskId)
s/selfURL/sourceURL/? or taskURL? self is ambiguous, i interpreted it as the URL
of the milo build page

[Ryan Tseng, 2017-03-02 03:15:53]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 03:16:04]

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, 2017-03-02 03:20:53]

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

[commit-bot: I haz the power, 2017-03-02 03:20:54]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/34a5d63270195810)

[Ryan Tseng, 2017-03-02 03:26:47]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 03:26:54]

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

[Ryan Tseng, 2017-03-02 03:31:36]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 03:31:44]

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, 2017-03-02 03:39:13]

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

[commit-bot: I haz the power, 2017-03-02 03:39:14]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-03-02 03:44:40]

https://codereview.chromium.org/2669473002/diff/40001/milo/api/resp/build.go
File milo/api/resp/build.go (right):

https://codereview.chromium.org/2669473002/diff/40001/milo/api/resp/build.go#...
milo/api/resp/build.go:222: Recipe *Link
On 2017/03/01 23:25:04, nodir wrote:
> add `json:,omitempty` to avoid all those `"Recipe": null` in the JSON
> expectation files

Done.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/frontend...
File
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html
(right):

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/expectations/buildbot-swarming.TestableBuild-build-canceled.html:83:
Recipe: <a
href="https://chromium.googlesource.com/chromium/tools/build/&#43;/master/">infra/infra_continuous</a>
On 2017/03/01 23:25:04, nodir wrote:
> this should be "+", not "&#43;"

go templates thinks &#43; is correct, and i tested it and it works here:
https://1561-0e72694-tainted-hinoka-dot-luci-milo.appspot.com/swarming/task/3...
+ in URLs translates to %20 usually

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:251: result := map[string]string{}
On 2017/03/01 23:25:04, nodir wrote:
> preallocate len(tags) entries

Done.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:253: if t == "" {
On 2017/03/01 23:25:04, nodir wrote:
> this if statement is unnecessary: strings.SpinN("", ":", 2) == []

Done.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:414: }
On 2017/03/01 23:25:04, nodir wrote:
> that's a lot of nesting. Consider inverting your if statements and put returns

Done.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:417: name := tags["recipe_name"]
On 2017/03/01 23:25:04, nodir wrote:
> name is not used

It's used as the label

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:425: subpath += "master/"
On 2017/03/01 23:25:04, nodir wrote:
> it is more correct to use HEAD, not master

HEAD doesn't make any sense on git if you don't specify a current branch or
commit.
See http://stackoverflow.com/questions/2304087/what-is-head-in-git
Also https://git-scm.com/book/en/v2/Git-Internals-Git-References
"The HEAD file is a symbolic reference to the branch you’re currently on"

/+/HEAD works probably because theres an implicit mapping that the current
branch is master, but that's an implicit thing rather than an explicit thing. 
It doesn't always imply the master branch.

We happen to use HEAD as a synonym to ToT on master because it is correct in
SVN.  The terminology carried over but it has a very subtly different meaning.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:429: URL:   fmt.Sprintf("%s/+/%s", repo,
subpath),
On 2017/03/01 23:25:04, nodir wrote:
> this works only on Gitiles. Let's not do this on non-gitiles. Please check if
> the repository looks like a Gitiles URL, e.g. ends with .googlesource.com

Done.

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:435: selfURL := taskPageURL(server, sr.TaskId)
On 2017/03/01 23:25:04, nodir wrote:
> s/selfURL/sourceURL/? or taskURL? self is ambiguous, i interpreted it as the
URL
> of the milo build page

Actually I'll just inline it.

[nodir, 2017-03-02 04:01:25]

lgtm % commens

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:417: name := tags["recipe_name"]
On 2017/03/02 03:44:40, hinoka wrote:
> On 2017/03/01 23:25:04, nodir wrote:
> > name is not used
> 
> It's used as the label

oh, sorry, i am blind

https://codereview.chromium.org/2669473002/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:425: subpath += "master/"
On 2017/03/02 03:44:40, hinoka wrote:
> On 2017/03/01 23:25:04, nodir wrote:
> > it is more correct to use HEAD, not master
> 
> HEAD doesn't make any sense on git if you don't specify a current branch or
> commit.
> See http://stackoverflow.com/questions/2304087/what-is-head-in-git
> Also https://git-scm.com/book/en/v2/Git-Internals-Git-References
> "The HEAD file is a symbolic reference to the branch you’re currently on"
> 
> /+/HEAD works probably because theres an implicit mapping that the current
> branch is master, but that's an implicit thing rather than an explicit thing. 
> It doesn't always imply the master branch.
> 
> We happen to use HEAD as a synonym to ToT on master because it is correct in
> SVN.  The terminology carried over but it has a very subtly different meaning.

ack

https://codereview.chromium.org/2669473002/diff/100001/milo/appengine/swarmin...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2669473002/diff/100001/milo/appengine/swarmin...
milo/appengine/swarming/build.go:429: if repoParse, err := url.Parse(repoURL);
err != nil && strings.HasSuffix(
err == nil

do you check your expectation files? :)

i assume something different is currently deployed to
https://1561-0e72694-tainted-hinoka-dot-luci-milo.appspot.com/swarming/task/3...

https://codereview.chromium.org/2669473002/diff/100001/milo/appengine/swarmin...
milo/appengine/swarming/build.go:430: repoParse.Host, "googlesource.com") {
prepend dot to googlesource.com

[Ryan Tseng, 2017-03-02 04:46:51]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 04:47:02]

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

[hinoka, 2017-03-02 04:47:41]

The CQ bit was unchecked by hinoka@chromium.org

[hinoka, 2017-03-02 04:47:47]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2017-03-02 04:47:47]

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/2669473002/#ps120001
(title: "Fix gerrit checker")

[commit-bot: I haz the power, 2017-03-02 04:47: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-03-02 04:55:50]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488430067092160,
"parent_rev": "22029efbdf1e0280e96ecc85efe3368d49c9f916", "commit_rev":
"d82ae617400d678575c2a8d877fcf1c9f2c44b03"}

[commit-bot: I haz the power, 2017-03-02 04:55:53]

Description was changed from

==========
Milo: Remove swarming tags from build properties

This makes the build view look cleaner.  This moves info from the swarming tag
onto other places like the execution and sourcestamp sections.

BUG=632898, 690591
==========

to

==========
Milo: Remove swarming tags from build properties

This makes the build view look cleaner.  This moves info from the swarming tag
onto other places like the execution and sourcestamp sections.

BUG=632898, 690591

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

[commit-bot: I haz the power, 2017-03-02 04:55:55]

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