Implement a server-side config service interface.

Implement a server-side config service interface.

This implements "server/config", a service-specific config service
interface. This interface is noticably different from the
"common/config.Interface" in the following ways:
- It accepts an authority: on whose behalf is the config request being
  issued?
- Defines a greatly-reduced interface, allowing for much simpler cache
  implementations.
- Allows for format translation, enabling implicit conversion and cache
  storage of configuration data in a different format from its native
  config service representation. Specifically, this targets storing text
  protobufs as binary.

Ultimately, we will delete the equivalent packages out of
"common/config", leaving it as a client endpoint to the config service.
Updates to services, notably "appengine/gaeconfig" will follow.

BUG=chromium:674378
TEST=unit

Review-Url: https://codereview.chromium.org/2580713002
Committed: https://github.com/luci/luci-go/commit/0e70e04e28394c34fad2ec3ba85ff402074f429e

[dnj, 2016-12-15 05:10:05]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-12-15 05:10:06]

PTAL. Full stack is:
https://codereview.chromium.org/2580713002 (this)
https://codereview.chromium.org/2578893002 (textproto)
https://codereview.chromium.org/2576993002 (access)
https://codereview.chromium.org/2576343002 (erroring)
https://codereview.chromium.org/2582433002 (test)
https://codereview.chromium.org/2573403002 (caching)
https://codereview.chromium.org/2575383002 (gaeconfig)
https://codereview.chromium.org/2576923003 (datastore cache)

[iannucci, 2017-01-07 20:12:16]

phew that was a long cl. anyway, lgtm

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority.go
File server/config/authority.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:1: // Copyright 2016 The LUCI Authors. All rights
reserved.
maybe 'luci-config' top level folder and maybe 'server/cfgclient' package name?

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:40: func (a Authority) String() string {
use stringer tool? https://godoc.org/golang.org/x/tools/cmd/stringer

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:76: return json.Marshal("U")
not sure it's worth calling json.Marshal here, constants (e.g. `[]byte("U")`)
would work fine.

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:83: func (a *Authority) UnmarshalJSON(d []byte) error
{
with constants mentioned above this becomes 

switch {
  case bytes.Equal(d, anonymousJSON):
  case bytes.Equal(d, userJSON):
  case bytes.Equal(d, serviceJSON):
  default:
}

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.go
File server/config/backend.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:19: // Content is the config item's content.
This comment and the one above are pretty self-evident. IMO I would just remove
them.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:27: // ForamtData is auxiliary data to supply to the
Formatter.
Format

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:31: // Params are backend parameters.
It would be helpful to know what these parameters are intended for, or where
they would be used or something like that. it looks like these are parameters
for a request?

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:37: Content bool
the other possibility would be a regexp-style collection of functions which
return the correct types. not sure what's better :/

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:43: Format string
would it be worth making a small type for this (e.g. `type Format string`) with
additional documentation on that type to clarify its role?

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:44: // FormatData is additional format data describing
the type. It may be empty.
I think this is opaque to this package and is passed along as-is to whatever
codes Format corresponds to?

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:48: // Backend is a configuration backend interface.
this is also pretty tautological :)

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go
File server/config/config.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:18: // configuration is not found.
I would mention that it's an alias of config.ErrNoConfig too (unless that shows
up in godoc. I forget)

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:47: func (gat *GetAllType) UnmarshalJSON(d []byte) error
{
same as other *JSON methods re: constant []byte values

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:81: // Get retrieves a single configuration.
can we call them 'configuration file's? Since from a program's pov, a single
configuration file may actually contain multiple application-specific
configurations.

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:101: item, err := be.Get(c, configSet, path, params)
maybe the Params thing would be clearer if backend were a different package? It
looks like backend is also internal so maybe params should be too? or can users
provide params as well?

I'm just being picky on names because we're really deep into the 'generic names'
bucket in the package. 'config.Params' is very nondescript :/

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go
File server/config/doc.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:1: // Copyright 2015 The LUCI Authors. All rights reserved.
also I wish that rietveld (or gerrit) let you somehow sort stuff
non-alphabetically so that all doc.go went to the top, all _test.go went to the
bottom, etc.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:9: // properties and data. The interface is designed to be
used with
'service-oriented'? which service? the config service, or the service someone is
implementeting?

can we `s/data/files` too? anything to provide additional specificity to the
discussion at hand.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:10: // service-appropriate caching layers. Config service
requests are made with
'service-appropriate'? same questions.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:23: //    format.
an application-specific cachable format

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:30: // the user to the underlying configuration data.
s/the user/the application/

since the application probably has its own notion of users, and we probably will
never have the users directly interface with this package

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go
File server/config/format.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go...
server/config/format.go:19: // formatter data
This is the format data that comes with the Params?

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go...
server/config/format.go:46: r.registry = make(map[string]Formatter)
nit: `map[string]Formatter{}` is shorter

https://codereview.chromium.org/2580713002/diff/20001/server/config/naming.go
File server/config/naming.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/naming.go...
server/config/naming.go:21: func ServiceConfigSet(name string) string { return
fmt.Sprintf("services/%s", name) }
do you think it would be worth having a small `type ConfigPath string` (or Name
or something) type for documentation centralization purposes?

[iannucci, 2017-01-07 20:55:51]

Oh, and if any of these comments are too annoying to do to the base CLs, they
can be done as followups on top of the full chain. (e.g. renaming the packages,
etc.)

[dnj, 2017-01-10 03:25:58]

Everything done, PTAL

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority.go
File server/config/authority.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:40: func (a Authority) String() string {
On 2017/01/07 20:12:15, iannucci wrote:
> use stringer tool? https://godoc.org/golang.org/x/tools/cmd/stringer

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:76: return json.Marshal("U")
On 2017/01/07 20:12:15, iannucci wrote:
> not sure it's worth calling json.Marshal here, constants (e.g. `[]byte("U")`)
> would work fine.

It would actually be []byte(`"U"`), which is a mistake that I've made before and
the reason I like calling Marshal :) But yeah that's fair, I'll just not make
the mistake. But you see my motivation!

https://codereview.chromium.org/2580713002/diff/20001/server/config/authority...
server/config/authority.go:83: func (a *Authority) UnmarshalJSON(d []byte) error
{
On 2017/01/07 20:12:15, iannucci wrote:
> with constants mentioned above this becomes 
> 
> switch {
>   case bytes.Equal(d, anonymousJSON):
>   case bytes.Equal(d, userJSON):
>   case bytes.Equal(d, serviceJSON):
>   default:
> }

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.go
File server/config/backend.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:19: // Content is the config item's content.
On 2017/01/07 20:12:15, iannucci wrote:
> This comment and the one above are pretty self-evident. IMO I would just
remove
> them.

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:27: // ForamtData is auxiliary data to supply to the
Formatter.
On 2017/01/07 20:12:15, iannucci wrote:
> Format

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:31: // Params are backend parameters.
On 2017/01/07 20:12:15, iannucci wrote:
> It would be helpful to know what these parameters are intended for, or where
> they would be used or something like that. it looks like these are parameters
> for a request?

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:37: Content bool
On 2017/01/07 20:12:15, iannucci wrote:
> the other possibility would be a regexp-style collection of functions which
> return the correct types. not sure what's better :/

I think this isn't bad. ATM it's explicit - add these filters, good to go.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:43: Format string
On 2017/01/07 20:12:15, iannucci wrote:
> would it be worth making a small type for this (e.g. `type Format string`)
with
> additional documentation on that type to clarify its role?

Did this, called it FormatSpec.

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:44: // FormatData is additional format data describing
the type. It may be empty.
On 2017/01/07 20:12:15, iannucci wrote:
> I think this is opaque to this package and is passed along as-is to whatever
> codes Format corresponds to?

Yep

https://codereview.chromium.org/2580713002/diff/20001/server/config/backend.g...
server/config/backend.go:48: // Backend is a configuration backend interface.
On 2017/01/07 20:12:15, iannucci wrote:
> this is also pretty tautological :)

Acknowledged.

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go
File server/config/config.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:18: // configuration is not found.
On 2017/01/07 20:12:16, iannucci wrote:
> I would mention that it's an alias of config.ErrNoConfig too (unless that
shows
> up in godoc. I forget)

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:47: func (gat *GetAllType) UnmarshalJSON(d []byte) error
{
On 2017/01/07 20:12:15, iannucci wrote:
> same as other *JSON methods re: constant []byte values

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:81: // Get retrieves a single configuration.
On 2017/01/07 20:12:16, iannucci wrote:
> can we call them 'configuration file's? Since from a program's pov, a single
> configuration file may actually contain multiple application-specific
> configurations.

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/config.go...
server/config/config.go:101: item, err := be.Get(c, configSet, path, params)
On 2017/01/07 20:12:15, iannucci wrote:
> maybe the Params thing would be clearer if backend were a different package?
It
> looks like backend is also internal so maybe params should be too? or can
users
> provide params as well?
> 
> I'm just being picky on names because we're really deep into the 'generic
names'
> bucket in the package. 'config.Params' is very nondescript :/

Yeah I was debating doing this. I think I was out of energy with the first path
set, but now that it's been a few I went ahead and split this out into "backend"
and "types".

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go
File server/config/doc.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:1: // Copyright 2015 The LUCI Authors. All rights reserved.
On 2017/01/07 20:12:16, iannucci wrote:
> also I wish that rietveld (or gerrit) let you somehow sort stuff
> non-alphabetically so that all doc.go went to the top, all _test.go went to
the
> bottom, etc.

Acknowledged.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:9: // properties and data. The interface is designed to be
used with
On 2017/01/07 20:12:16, iannucci wrote:
> 'service-oriented'? which service? the config service, or the service someone
is
> implementeting?
> 
> can we `s/data/files` too? anything to provide additional specificity to the
> discussion at hand.

Done. The entire "server/" package has this theme, it seems odd to specifically
have this package define it.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:10: // service-appropriate caching layers. Config service
requests are made with
On 2017/01/07 20:12:16, iannucci wrote:
> 'service-appropriate'? same questions.

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:23: //    format.
On 2017/01/07 20:12:16, iannucci wrote:
> an application-specific cachable format

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/doc.go#ne...
server/config/doc.go:30: // the user to the underlying configuration data.
On 2017/01/07 20:12:16, iannucci wrote:
> s/the user/the application/
> 
> since the application probably has its own notion of users, and we probably
will
> never have the users directly interface with this package

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go
File server/config/format.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go...
server/config/format.go:19: // formatter data
On 2017/01/07 20:12:16, iannucci wrote:
> This is the format data that comes with the Params?

It's the data in Item's Content, implemented by the FormatterBackend.

https://codereview.chromium.org/2580713002/diff/20001/server/config/format.go...
server/config/format.go:46: r.registry = make(map[string]Formatter)
On 2017/01/07 20:12:16, iannucci wrote:
> nit: `map[string]Formatter{}` is shorter

Done.

https://codereview.chromium.org/2580713002/diff/20001/server/config/naming.go
File server/config/naming.go (right):

https://codereview.chromium.org/2580713002/diff/20001/server/config/naming.go...
server/config/naming.go:21: func ServiceConfigSet(name string) string { return
fmt.Sprintf("services/%s", name) }
On 2017/01/07 20:12:16, iannucci wrote:
> do you think it would be worth having a small `type ConfigPath string` (or
Name
> or something) type for documentation centralization purposes?

Yep, made "luci_config/common/cfgtypes" for this, switched everything over.

[dnj, 2017-01-10 23:01:51]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-01-10 23:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2580713002/#ps40001
(title: "Renamed, moved to luci_config package, fixes, split out backends.")

[commit-bot: I haz the power, 2017-01-10 23:02:00]

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-01-10 23:07:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484089311833250,
"parent_rev": "642108c30d02d7384b5bd960e470a8b4e0ef045b", "commit_rev":
"0e70e04e28394c34fad2ec3ba85ff402074f429e"}

[commit-bot: I haz the power, 2017-01-10 23:07:40]

Description was changed from

==========
Implement a server-side config service interface.

This implements "server/config", a service-specific config service
interface. This interface is noticably different from the
"common/config.Interface" in the following ways:
- It accepts an authority: on whose behalf is the config request being
  issued?
- Defines a greatly-reduced interface, allowing for much simpler cache
  implementations.
- Allows for format translation, enabling implicit conversion and cache
  storage of configuration data in a different format from its native
  config service representation. Specifically, this targets storing text
  protobufs as binary.

Ultimately, we will delete the equivalent packages out of
"common/config", leaving it as a client endpoint to the config service.
Updates to services, notably "appengine/gaeconfig" will follow.

BUG=chromium:674378
TEST=unit
==========

to

==========
Implement a server-side config service interface.

This implements "server/config", a service-specific config service
interface. This interface is noticably different from the
"common/config.Interface" in the following ways:
- It accepts an authority: on whose behalf is the config request being
  issued?
- Defines a greatly-reduced interface, allowing for much simpler cache
  implementations.
- Allows for format translation, enabling implicit conversion and cache
  storage of configuration data in a different format from its native
  config service representation. Specifically, this targets storing text
  protobufs as binary.

Ultimately, we will delete the equivalent packages out of
"common/config", leaving it as a client endpoint to the config service.
Updates to services, notably "appengine/gaeconfig" will follow.

BUG=chromium:674378
TEST=unit

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

[commit-bot: I haz the power, 2017-01-10 23:07:41]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/0e70e04e28394c34fad2ec3ba85ff402074f429e