control-plane: introduce a client for Konvoy REST API in the form of "remote" ResourceStore

[yskopets]

Context:

• konvoyctl must be able to request and unmarshal a list of resources from Konvoy API Server

Changes:

• introduce model.ResourceRegistry abstraction to instantiate a resource by its ResourceType
• introduce rest.ResourceListReceiver type to unmarshal into rest.ResourceList
• introduce rest.Mapper abstraction to describe REST representation of a resource
• introduce a client for Konvoy API Server in the form of "remote" ResourceStore

[jakubdyszkiewicz]

Why do we need this Mapper abstraction? Why just pass there WsDefinitions and use them internally.
Why this is an interface? What other implementation do you expect to see?

btw. Mapper don't tell me much. What does it maps? How about ResourcePaths. ResourcePaths.get(resource).

[yskopets]

WsDefinitions is a server-side description of the API, rest.Mapper is a client-side counterpart.

Between plain map[model.ResourceType]ResourceMapping and func(model.ResourceType) (ResourceMapping, error) I chose the latter to avoid inconsistent handling of missing types by client code.

And then it's a design principle to promote a function into a single method interface.

[jakubdyszkiewicz]

WsDefinitions is a server-side description of the API

It's server side, but it's used for client anyways, just after one layer.

inconsistent handling of missing types by client code

where this incosistency can occur?
func(model.ResourceType) (ResourceMapping, error) can be just a method of remoteStore that uses the map.

I'm fine with this mapper, but please rename it to more descriptive name like ResourcePaths

[yskopets]

rest.Mapper was renamed into rest.Api

[jakubdyszkiewicz]

This is question for the whole codebase. Why do we name the variables with such a short name?
I get that receiver s *remoteStore is short, because it's used all the time (although I'd prefer this, but this seems not to be go way...), but
rsr, b, ri, r? Why not listReceiver, body, listItem, item?
Sometimes is hard to follow and I need to go back to variable declaration to remember what was that.

[yskopets]

"idiomatic golang"

[jakubdyszkiewicz]

I like to be pragmatic. If convention hurts readability then it's maybe not the best one.
Clean code (descriptive names) can be applied no mater what is the languages. I'm not a fan of superLongNamedFromTheJavaWorld, but I think there should be a middle ground somewhere.

[jakubdyszkiewicz]

what does cr stands for?

[yskopets]

core resource

[jakubdyszkiewicz]

Why do we need a ResourceRegistry with interface and impl if all we support is one type, that we already know. Why don't we just pass function to ResourceListReceiver for creating a new item.

[yskopets]

ResourceRegistry is an interface for creating a new item of any supported type.

A function can always be wrapped into ResourceRegistry (e.g., http.Handler and http.HandlerFunc).

I could replace in-place instantiation ( &model.SimpleResourceRegistry{ ... } ) with a factory method, e.g. SingleTypeRegistry(model.ResourceList)

[jakubdyszkiewicz]

I understand what it is, but why do we need it? Why not just have

type ResourceListReceiver struct {
	ResourceList
        NewResource func() Resource
}

I think that Go code strives to be simpler without unnecessary abstractions which is common in Java.

[yskopets]

Changed to

type ResourceListReceiver struct {
	ResourceList
        NewResource func() Resource
}