Milo: Print raw json for buildbot build properties

Milo: Print raw json for buildbot build properties

Also remove the buildbucket -> URL conversion.

BUG=687291
Review-Url: https://codereview.chromium.org/2718373004
Committed: https://github.com/luci/luci-go/commit/0a0dba06054fdac1c08b08676414f6cf1188c8a8

[Ryan Tseng, 2017-02-28 18:49:33]

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

[commit-bot: I haz the power, 2017-02-28 18:49: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-02-28 18:51:04]

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

[commit-bot: I haz the power, 2017-02-28 18:51:12]

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

[hinoka, 2017-02-28 18:51:49]

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

[hinoka, 2017-02-28 18:51:49]

ptal

[commit-bot: I haz the power, 2017-02-28 18:58:42]

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

[commit-bot: I haz the power, 2017-02-28 18:58:43]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-03-22 21:57:49]

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

[commit-bot: I haz the power, 2017-03-22 21:57:59]

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-22 22:04:39]

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

[commit-bot: I haz the power, 2017-03-22 22:04:40]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-03-22 22:08:26]

I rebased this, here is what it looks like now

https://1619-cca3eb7-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/tr...

I forgot what the concerns were.  We wanted properties to look like a json dump?

[nodir, 2017-03-24 06:05:40]

My primary concern was that it is impossible to unambiguously tell a property
value type by looking at the value. The types are important because users write
code that process those values, it is not only for human consumption.

Last time we talked, the solution the problem was to present all values (not
only JSON objects) in JSON format.

We also considered to paint double quotes of strings as gray to make them not
stand out, but I am not sure we want that because we will probably won't be
painting double-quotes in ["blah"] as gray. I think I'd leave double quotes as
is for simplicity.

https://codereview.chromium.org/2718373004/diff/20001/milo/appengine/settings...
File milo/appengine/settings/funcs.go (right):

https://codereview.chromium.org/2718373004/diff/20001/milo/appengine/settings...
milo/appengine/settings/funcs.go:31: "formatJson":     formatJson,
s/formatJson/formatJSON/g

https://codereview.chromium.org/2718373004/diff/20001/milo/appengine/settings...
milo/appengine/settings/funcs.go:192: func formatJson(s string) template.HTML {
formatJSON

In Go acronyms are written all caps

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/common/f...
File milo/appengine/common/funcs.go (right):

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/common/f...
milo/appengine/common/funcs.go:209: err := json.Unmarshal([]byte(s), &o)
here we parse a JSON-formatted value and then conditionally format to JSON
back.. Earlier in the code path with had this value in parsed form, so we did
(parsed -> JSON -> parsed -> JSON) conversion. This is wasteful

Please change formatJson to accept an interface{} and treat the passed value as
already parsed.

Also please rename formatJson to formatJSON because Go likes abbreviation
written with all caps

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/common/f...
milo/appengine/common/funcs.go:217: s, err := json.MarshalIndent(t, "", "  ")
AFAIR last time we talked about this I asked to render property values of any
type in JSON format, so that a user can clearly see the value type. It is
imporatnt to know the type because users write code that works with the values.
Today it is ambiguous, value `1` may be a string "1" or a number 1. Print only
structs as JSON but other types is confusing. A person who sees a JSON object
presented as valid JSON may reasonably assume that in general milo presents
values in JSON format.

Please format all values in JSON format.

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/frontend...
File
milo/appengine/frontend/expectations/bootstrap-buildbot.TestableBuild-Debug_page-_win_chromium_rel_ng_246309.html
(right):

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/frontend...
milo/appengine/frontend/expectations/bootstrap-buildbot.TestableBuild-Debug_page-_win_chromium_rel_ng_246309.html:1:

is this file here by mistake? I see "bootstrap" in its name

[nodir, 2017-03-24 08:46:06]

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/common/f...
File milo/appengine/common/funcs.go (right):

https://codereview.chromium.org/2718373004/diff/40001/milo/appengine/common/f...
milo/appengine/common/funcs.go:217: s, err := json.MarshalIndent(t, "", "  ")
On 2017/03/24 06:05:40, nodir wrote:
> AFAIR last time we talked about this I asked to render property values of any
> type in JSON format, so that a user can clearly see the value type. It is
> imporatnt to know the type because users write code that works with the
values.
> Today it is ambiguous, value `1` may be a string "1" or a number 1. Print only
> structs as JSON but other types is confusing. A person who sees a JSON object
> presented as valid JSON may reasonably assume that in general milo presents
> values in JSON format.
> 
> Please format all values in JSON format.

Obviously please update the code that is currently encodes values in
JSON before passing them to build page template. Make it pass raw typed values.

[nodir, 2017-04-07 17:36:50]

what's the status of this CL

[Ryan Tseng, 2017-04-10 18:45:48]

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

[commit-bot: I haz the power, 2017-04-10 18:45:59]

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-04-10 18:53:08]

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

[commit-bot: I haz the power, 2017-04-10 18:53:09]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-04-10 19:35:01]

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

[commit-bot: I haz the power, 2017-04-10 19:35:10]

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

[hinoka, 2017-04-10 19:37:40]

Description was changed from

==========
Milo: Pretty print JSON build properties.

Also remove the buildbucket -> URL conversion.

BUG=687291
==========

to

==========
Milo: Print raw json for build properties

Also remove the buildbucket -> URL conversion.

BUG=687291
==========

[hinoka, 2017-04-10 19:37:57]

This is updated to not pretty-print

See
https://1656-4b9e7d5-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/tr...

[commit-bot: I haz the power, 2017-04-10 19:41:21]

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

[commit-bot: I haz the power, 2017-04-10 19:41:22]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-04-11 07:13:20]

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:317: // parseProp returns a representation of v
based off k, and a boolean to
there is no k

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:317: // parseProp returns a representation of v
based off k, and a boolean to
high level: i think JSON encoding should be in a template func, not here
because this logic is a part of presentation

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:318: // specify whether or not to hide it
altogether.
after I read this, i thought the function returns true if a property should be
hidden. 
maybe "whether or not to display it"? or more explicit ", and false to specify
to hide it altogether"

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:321: return "", false // Value is empty, don't
show it.
why we are hiding empty strings? They are legit values

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:373: Value: vs,
i think this should be an interface{} and JSON-encoding should be done via a
func in a template

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/frontend...
File milo/appengine/frontend/expectations/swarming.build-build-canceled.html
(right):

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/frontend...
milo/appengine/frontend/expectations/swarming.build-build-canceled.html:165:
<td>kitchen</td>
why these are not JSON?

[nodir, 2017-04-11 07:14:42]

see my high level comment 2w ago:

"please update the code that is currently encodes values in
JSON before passing them to build page template. Make it pass raw typed values"

this wasn't done

[nodir, 2017-04-11 07:34:19]

On 2017/04/11 07:14:42, nodir wrote:
> see my high level comment 2w ago:
> 
> "please update the code that is currently encodes values in
> JSON before passing them to build page template. Make it pass raw typed
values"
> 
> this wasn't done

Actually this is fine. Let's leave it as is

Lgtm % empty strings. I think we should not hide them

[hinoka, 2017-04-13 21:00:29]

Description was changed from

==========
Milo: Print raw json for build properties

Also remove the buildbucket -> URL conversion.

BUG=687291
==========

to

==========
Milo: Print raw json for buildbot build properties

Also remove the buildbucket -> URL conversion.

BUG=687291
==========

[Ryan Tseng, 2017-04-13 21:01:52]

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

[commit-bot: I haz the power, 2017-04-13 21:01:55]

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

[hinoka, 2017-04-13 21:01:59]

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:317: // parseProp returns a representation of v
based off k, and a boolean to
On 2017/04/11 07:13:19, nodir wrote:
> high level: i think JSON encoding should be in a template func, not here
> because this logic is a part of presentation

This was done this was because previously rietveld links required knowing both
the "rietveld" and "issue" fields (to match buildbot behavior).  This has been
since moved to the "Source" section.

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:318: // specify whether or not to hide it
altogether.
On 2017/04/11 07:13:19, nodir wrote:
> after I read this, i thought the function returns true if a property should be
> hidden. 
> maybe "whether or not to display it"? or more explicit ", and false to specify
> to hide it altogether"

Removed block.

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:321: return "", false // Value is empty, don't
show it.
On 2017/04/11 07:13:19, nodir wrote:
> why we are hiding empty strings? They are legit values

They why is because it seems kind of useless to display them.  But if you think
they should be there i can put it back.

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:373: Value: vs,
On 2017/04/11 07:13:20, nodir wrote:
> i think this should be an interface{} and JSON-encoding should be done via a
> func in a template

I'll do that in a different CL.  That would undermine a design decision that was
made in Milo. (ie, that properties are mean to be json serializable values meant
for machine consumption rather than human use.)

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/frontend...
File milo/appengine/frontend/expectations/swarming.build-build-canceled.html
(right):

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/frontend...
milo/appengine/frontend/expectations/swarming.build-build-canceled.html:165:
<td>kitchen</td>
On 2017/04/11 07:13:20, nodir wrote:
> why these are not JSON?

This CL is buildbot only.  Updated description.

[hinoka, 2017-04-13 21:02:07]

The CQ bit was unchecked by hinoka@chromium.org

[hinoka, 2017-04-13 21:02:10]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2017-04-13 21:02:11]

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/2718373004/#ps100001
(title: "Add in empty strings")

[commit-bot: I haz the power, 2017-04-13 21:02:17]

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

[nodir, 2017-04-13 21:08:20]

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
File milo/appengine/buildbot/build.go (right):

https://codereview.chromium.org/2718373004/diff/80001/milo/appengine/buildbot...
milo/appengine/buildbot/build.go:321: return "", false // Value is empty, don't
show it.
On 2017/04/13 21:01:59, hinoka wrote:
> On 2017/04/11 07:13:19, nodir wrote:
> > why we are hiding empty strings? They are legit values
> 
> They why is because it seems kind of useless to display them.  But if you
think
> they should be there i can put it back.

Yeah, I think they should be there. Imagine someone triggers a build a
specifies a="" property. They would think they are doing something wrong if they
don't see the property in the list

[nodir, 2017-04-13 21:08:34]

The CQ bit was unchecked by nodir@chromium.org

[nodir, 2017-04-13 21:08:56]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2017-04-13 21:09:03]

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-04-13 21:15:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492117736267280,
"parent_rev": "3e4d258fcd015200bdc87eba6ad5ff1d0a611ea8", "commit_rev":
"0a0dba06054fdac1c08b08676414f6cf1188c8a8"}

[commit-bot: I haz the power, 2017-04-13 21:15:53]

Description was changed from

==========
Milo: Print raw json for buildbot build properties

Also remove the buildbucket -> URL conversion.

BUG=687291
==========

to

==========
Milo: Print raw json for buildbot build properties

Also remove the buildbucket -> URL conversion.

BUG=687291

Review-Url: https://codereview.chromium.org/2718373004
Committed:
https://github.com/luci/luci-go/commit/0a0dba06054fdac1c08b08676414f6cf1188c8a8
==========

[commit-bot: I haz the power, 2017-04-13 21:15:55]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/0a0dba06054fdac1c08b08676414f6cf1188c8a8