Milo: Add ?limit= support to builders

Milo: Add ?limit= support to builders

So you can see more than 25 builds

BUG=654623
Committed: https://github.com/luci/luci-go/commit/d7f554b78efddab387c85fd80e89410011a35683

[Ryan Tseng, 2016-09-08 20:52:28]

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

[commit-bot: I haz the power, 2016-09-08 20:52:39]

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

[Ryan Tseng, 2016-09-08 20:52:48]

hinoka@google.com changed reviewers:
+ estaab@chromium.org

[Ryan Tseng, 2016-09-08 20:52:48]

ptal

demo:
https://1103-e1a482a-tainted-hinoka-dot-luci-milo.appspot.com/buildbot/tryser...

[commit-bot: I haz the power, 2016-09-08 21:00:25]

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

[commit-bot: I haz the power, 2016-09-08 21:00:26]

Dry run: This issue passed the CQ dry run.

[hinoka, 2016-10-27 01:35:19]

Description was changed from

==========
Milo: Add ?limit= support to builders

So you can see more than 25 builds

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

to

==========
Milo: Add ?limit= support to builders

So you can see more than 25 builds

BUG=654623
==========

[Ryan Tseng, 2016-10-27 01:35:57]

Rebase

[Ryan Tseng, 2016-10-27 01:35:57]

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

[commit-bot: I haz the power, 2016-10-27 01:36:08]

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

[hinoka, 2016-10-27 01:36:11]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org, martiniss@chromium.org, nodir@chromium.org
- estaab@chromium.org

[hinoka, 2016-10-27 01:36:12]

ptal, the current build CL is dependent on this one

[commit-bot: I haz the power, 2016-10-27 01:39:38]

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

[commit-bot: I haz the power, 2016-10-27 01:39:39]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/321c9964d50d0d10)

[martiniss, 2016-10-27 02:23:16]

Code looks fine, but the demo page doesn't work. 

Also, it's not showing me the name of the builder...

[hinoka, 2016-10-27 02:31:15]

Builder name is this CL: https://codereview.chromium.org/2407153006

Heres another demo:
https://1197-8dd2fd9-tainted-hinoka-dot-luci-milo.appspot.com/buildbot/tryser...

[nodir, 2016-10-28 07:17:54]

lgtm

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/html.go (right):

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:53: // limit is a name of the query string
parameter for specifying
this function does not know the name of the query string parameter, GetLimit
encapsulated it

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

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:157: func GetLimit(r *http.Request) (int,
error) {
why this is in settings package? what does limit query string parameter have to
do with settings?

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:158: sLimit := r.FormValue("limit")
can you also fall back to "numBuilds"? this is what buildbucket builder view
uses because buildbot uses it

[Ryan Tseng, 2016-10-28 19:43:36]

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

[commit-bot: I haz the power, 2016-10-28 19:43:47]

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

[Ryan Tseng, 2016-10-28 19:47:09]

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

[commit-bot: I haz the power, 2016-10-28 19:47:18]

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

[hinoka, 2016-10-28 19:47:30]

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/buildbuc...
File milo/appengine/buildbucket/html.go (right):

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/buildbuc...
milo/appengine/buildbucket/html.go:53: // limit is a name of the query string
parameter for specifying
On 2016/10/28 07:17:54, nodir wrote:
> this function does not know the name of the query string parameter, GetLimit
> encapsulated it

Removed...

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

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:157: func GetLimit(r *http.Request) (int,
error) {
On 2016/10/28 07:17:54, nodir wrote:
> why this is in settings package? what does limit query string parameter have
to
> do with settings?

Technically nothing, i'm using the settings package as sort of a "misc" or
"utils" package, since it has to be imported everywhere anyways.  I could split
it out, but I feel like theres too many top-level packages already, and that
would only complicate things.

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:158: sLimit := r.FormValue("limit")
On 2016/10/28 07:17:54, nodir wrote:
> can you also fall back to "numBuilds"? this is what buildbucket builder view
> uses because buildbot uses it

Done.

[nodir, 2016-10-28 19:52:51]

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

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:157: func GetLimit(r *http.Request) (int,
error) {
On 2016/10/28 19:47:30, hinoka wrote:
> On 2016/10/28 07:17:54, nodir wrote:
> > why this is in settings package? what does limit query string parameter have
> to
> > do with settings?
> 
> Technically nothing, i'm using the settings package as sort of a "misc" or
> "utils" package, since it has to be imported everywhere anyways.  I could
split
> it out, but I feel like theres too many top-level packages already, and that
> would only complicate things.

Perhaps this needs some refactoring. Go does not encourage misc/utils packages.

[hinoka, 2016-10-28 19:53:42]

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

https://codereview.chromium.org/2328733002/diff/20001/milo/appengine/settings...
milo/appengine/settings/settings.go:157: func GetLimit(r *http.Request) (int,
error) {
On 2016/10/28 19:52:51, nodir wrote:
> On 2016/10/28 19:47:30, hinoka wrote:
> > On 2016/10/28 07:17:54, nodir wrote:
> > > why this is in settings package? what does limit query string parameter
have
> > to
> > > do with settings?
> > 
> > Technically nothing, i'm using the settings package as sort of a "misc" or
> > "utils" package, since it has to be imported everywhere anyways.  I could
> split
> > it out, but I feel like theres too many top-level packages already, and that
> > would only complicate things.
> 
> Perhaps this needs some refactoring. Go does not encourage misc/utils
packages.

The alternative would be copypasta code...?

[commit-bot: I haz the power, 2016-10-28 19:55:06]

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

[commit-bot: I haz the power, 2016-10-28 19:55:07]

Dry run: This issue passed the CQ dry run.

[hinoka, 2016-10-28 20:00:22]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2016-10-28 20:00:23]

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/2328733002/#ps60001
(title: "Regenerate")

[commit-bot: I haz the power, 2016-10-28 20:00:27]

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 20:04:51]

Description was changed from

==========
Milo: Add ?limit= support to builders

So you can see more than 25 builds

BUG=654623
==========

to

==========
Milo: Add ?limit= support to builders

So you can see more than 25 builds

BUG=654623

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

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

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/d7f554b78efddab387c85fd80e89410011a35683