Milo: Use luci-config for storing buildbot acls

Milo: Use luci-config for storing buildbot acls

This moves buildbot acls to be checked from luci config rather than a magical
project named "chrome"

BUG=700055
Review-Url: https://codereview.chromium.org/2760873003
Committed: https://github.com/luci/luci-go/commit/12104d6afc53cde67e7572a7b32ff062adaa2f0e

[Ryan Tseng, 2017-03-20 20:34:16]

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

[commit-bot: I haz the power, 2017-03-20 20:34:19]

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-20 20:35:48]

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

[hinoka, 2017-03-20 20:35:49]

ptal

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/frontend/con...
File milo/appengine/frontend/config.go (right):

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/frontend/con...
milo/appengine/frontend/config.go:33: func UpdateConfigHandler(ctx
*router.Context) {
Addressing leftover comments from the last CL

[commit-bot: I haz the power, 2017-03-20 20:41:08]

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

[commit-bot: I haz the power, 2017-03-20 20:41:09]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-03-20 21:41:32]

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/acl.go
File milo/appengine/common/acl.go (right):

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/acl.g...
milo/appengine/common/acl.go:47: return false, errMissingReaders
is it an error though? I think it just means that only admins can this

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
File milo/appengine/common/config.go (right):

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:41: err := cfgclient.Get(c, cfgclient.AsService,
cs, "settings.cfg", cfgclient.String(&content), nil)
this API call should be used in serving path. LuciConfig has very low
reliability and should be used only in background tasks.

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:44: return nil, errNotConfigured
this will cause http500s on all requests, I assume?
Why not return an empty Settings struct?

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:51: err = proto.UnmarshalText(content, msg)
make sure the config is stored in binary so we can rollback milo version

https://codereview.chromium.org/2760873003/diff/1/milo/common/config/settings...
File milo/common/config/settings.proto (right):

https://codereview.chromium.org/2760873003/diff/1/milo/common/config/settings...
milo/common/config/settings.proto:13: string buildbot_internal_reader = 1;
presumably we'll have more buildbot-specific settings.
please make a message Buildbot and put internal_reader there.

[Ryan Tseng, 2017-03-20 22:18:56]

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

[commit-bot: I haz the power, 2017-03-20 22:18:58]

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-20 22:19:28]

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/acl.go
File milo/appengine/common/acl.go (right):

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/acl.g...
milo/appengine/common/acl.go:47: return false, errMissingReaders
On 2017/03/20 21:41:32, nodir wrote:
> is it an error though? I think it just means that only admins can this

Done.

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
File milo/appengine/common/config.go (right):

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:41: err := cfgclient.Get(c, cfgclient.AsService,
cs, "settings.cfg", cfgclient.String(&content), nil)
On 2017/03/20 21:41:32, nodir wrote:
> this API call should be used in serving path. LuciConfig has very low
> reliability and should be used only in background tasks.

Done.

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:44: return nil, errNotConfigured
On 2017/03/20 21:41:32, nodir wrote:
> this will cause http500s on all requests, I assume?
> Why not return an empty Settings struct?

Done.

https://codereview.chromium.org/2760873003/diff/1/milo/appengine/common/confi...
milo/appengine/common/config.go:51: err = proto.UnmarshalText(content, msg)
On 2017/03/20 21:41:32, nodir wrote:
> make sure the config is stored in binary so we can rollback milo version

Done.

https://codereview.chromium.org/2760873003/diff/1/milo/common/config/settings...
File milo/common/config/settings.proto (right):

https://codereview.chromium.org/2760873003/diff/1/milo/common/config/settings...
milo/common/config/settings.proto:13: string buildbot_internal_reader = 1;
On 2017/03/20 21:41:32, nodir wrote:
> presumably we'll have more buildbot-specific settings.
> please make a message Buildbot and put internal_reader there.

This seems a little opportunistic, but sure.  Done.

[commit-bot: I haz the power, 2017-03-20 22:26:08]

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

[commit-bot: I haz the power, 2017-03-20 22:26:09]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-03-21 00:55:33]

Is this okay now?

[nodir, 2017-03-21 01:06:52]

lgtm % comments

https://codereview.chromium.org/2760873003/diff/20001/milo/appengine/common/c...
File milo/appengine/common/config.go (right):

https://codereview.chromium.org/2760873003/diff/20001/milo/appengine/common/c...
milo/appengine/common/config.go:45: return msg, nil
err = nil

in case we add more code at L51

https://codereview.chromium.org/2760873003/diff/20001/milo/common/config/sett...
File milo/common/config/settings.proto (right):

https://codereview.chromium.org/2760873003/diff/20001/milo/common/config/sett...
milo/common/config/settings.proto:14: string buildbot_internal_reader = 1;
s/buildbot_internal_reader/internal_reader/

[Ryan Tseng, 2017-03-21 04:53:52]

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

[commit-bot: I haz the power, 2017-03-21 04:53:57]

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-21 04:54:53]

https://codereview.chromium.org/2760873003/diff/20001/milo/appengine/common/c...
File milo/appengine/common/config.go (right):

https://codereview.chromium.org/2760873003/diff/20001/milo/appengine/common/c...
milo/appengine/common/config.go:45: return msg, nil
On 2017/03/21 01:06:51, nodir wrote:
> err = nil
> 
> in case we add more code at L51

Done.

https://codereview.chromium.org/2760873003/diff/20001/milo/common/config/sett...
File milo/common/config/settings.proto (right):

https://codereview.chromium.org/2760873003/diff/20001/milo/common/config/sett...
milo/common/config/settings.proto:14: string buildbot_internal_reader = 1;
On 2017/03/21 01:06:51, nodir wrote:
> s/buildbot_internal_reader/internal_reader/

Oh right, of course.  Done

[commit-bot: I haz the power, 2017-03-21 04:58:15]

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

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

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

[Ryan Tseng, 2017-03-21 05:00:19]

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

[commit-bot: I haz the power, 2017-03-21 05:00:25]

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-21 05:05:16]

The CQ bit was unchecked by hinoka@chromium.org

[hinoka, 2017-03-21 05:05:21]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2017-03-21 05:05:22]

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/2760873003/#ps60001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-03-21 05:05:24]

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-21 05:11:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490072721823310,
"parent_rev": "f3bc112bcbeb968cf79bcc8766d759bb65538a53", "commit_rev":
"12104d6afc53cde67e7572a7b32ff062adaa2f0e"}

[commit-bot: I haz the power, 2017-03-21 05:11:46]

Description was changed from

==========
Milo: Use luci-config for storing buildbot acls

This moves buildbot acls to be checked from luci config rather than a magical
project named "chrome"

BUG=700055
==========

to

==========
Milo: Use luci-config for storing buildbot acls

This moves buildbot acls to be checked from luci config rather than a magical
project named "chrome"

BUG=700055

Review-Url: https://codereview.chromium.org/2760873003
Committed:
https://github.com/luci/luci-go/commit/12104d6afc53cde67e7572a7b32ff062adaa2f0e
==========

[commit-bot: I haz the power, 2017-03-21 05:11:47]

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