Adds checking of template error

Adds checking of template error

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/2630b8577d7c6ca79f46b7bed7464fec16102f23

[emso, 2016-07-14 06:47:28]

Description was changed from

==========
Adds checking of template error

BUG=
==========

to

==========
Adds checking of template error

BUG=
==========

[emso, 2016-07-14 06:47:28]

emso@chromium.org changed reviewers:
+ nodir@chromium.org

[nodir, 2016-07-14 16:33:00]

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
File go/src/infra/tricium/service/default/handlers.go (right):

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
go/src/infra/tricium/service/default/handlers.go:28: if e := basePage.Execute(w,
data); e != nil {
I recommend calling the variable "err" because it is pretty standard across all
Go code I saw

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
go/src/infra/tricium/service/default/handlers.go:29: http.Error(w, e.Error(),
http.StatusInternalServerError)
the error description is sent to the user and not logged. If this happens, it
will be hard for you to analyze the error because you won't know what was the
error.
I'd log it. Also consider sending "Internal server error, we are working on it"
or something like that to the user, so you don't expose implementation details
to the user.

Also consider writing a helper function that executes a template to response, so
you don't have to copy this code everywhere

[emso, 2016-07-15 11:04:44]

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
File go/src/infra/tricium/service/default/handlers.go (right):

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
go/src/infra/tricium/service/default/handlers.go:28: if e := basePage.Execute(w,
data); e != nil {
On 2016/07/14 16:33:00, nodir wrote:
> I recommend calling the variable "err" because it is pretty standard across
all
> Go code I saw

Done.

https://codereview.chromium.org/2149853003/diff/1/go/src/infra/tricium/servic...
go/src/infra/tricium/service/default/handlers.go:29: http.Error(w, e.Error(),
http.StatusInternalServerError)
On 2016/07/14 16:33:00, nodir wrote:
> the error description is sent to the user and not logged. If this happens, it
> will be hard for you to analyze the error because you won't know what was the
> error.
> I'd log it. Also consider sending "Internal server error, we are working on
it"
> or something like that to the user, so you don't expose implementation details
> to the user.
> 
> Also consider writing a helper function that executes a template to response,
so
> you don't have to copy this code everywhere

Done.

[nodir, 2016-07-15 16:38:23]

lgtm

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
File go/src/infra/tricium/service/common/common.go (right):

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
go/src/infra/tricium/service/common/common.go:24: return fmt.Errorf("Error
executing template: %v", err)
error messages should not start with  a capital letter, because it is typical to
prepend more context text to an error message, like here for example

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
File go/src/infra/tricium/service/default/handlers.go (right):

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
go/src/infra/tricium/service/default/handlers.go:14: common
"infra/tricium/service/common"
nit: not sure why you put `common ` before path. It seems to rename package
"common" to "common"

[emso, 2016-07-19 08:38:44]

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
File go/src/infra/tricium/service/common/common.go (right):

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
go/src/infra/tricium/service/common/common.go:24: return fmt.Errorf("Error
executing template: %v", err)
On 2016/07/15 16:38:23, nodir wrote:
> error messages should not start with  a capital letter, because it is typical
to
> prepend more context text to an error message, like here for example

Done.

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
File go/src/infra/tricium/service/default/handlers.go (right):

https://codereview.chromium.org/2149853003/diff/20001/go/src/infra/tricium/se...
go/src/infra/tricium/service/default/handlers.go:14: common
"infra/tricium/service/common"
On 2016/07/15 16:38:23, nodir wrote:
> nit: not sure why you put `common ` before path. It seems to rename package
> "common" to "common"

Not thinking. Removing. Thanks.

[emso, 2016-07-20 08:36:56]

The CQ bit was checked by emso@chromium.org

[emso, 2016-07-20 08:36:57]

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/2149853003/#ps40001
(title: "Cleans up message and package use")

[commit-bot: I haz the power, 2016-07-20 08:37:07]

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-07-20 08:43:56]

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

[commit-bot: I haz the power, 2016-07-20 08:43:56]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)
  Infra Win Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

[emso, 2016-07-25 13:44:32]

The CQ bit was checked by emso@chromium.org

[emso, 2016-07-25 13:44:32]

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/2149853003/#ps60001
(title: "Adds infra testing expectation")

[commit-bot: I haz the power, 2016-07-25 13:44:39]

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-07-25 13:59:55]

Description was changed from

==========
Adds checking of template error

BUG=
==========

to

==========
Adds checking of template error

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/2630b8577d7c6ca79f46b7bed7464...
==========

[commit-bot: I haz the power, 2016-07-25 13:59:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/2630b8577d7c6ca79f46b7bed7464...