Proposal/refactor: Drop package level funcs in favor of Newable Go types and Improve package APIs

[ilgooz]

Current implementation of core code should be refactored with more idiomatic Go code. Changes will make unit testing easier as a side effect which is the second purpose of this proposal.

We need to drop package level functions and create Go types for most of the core packages to attach them as their methods. Converting package level functions to type methods gives a nice isolation, grouped APIs and overall look. It also helps with testing to pass mocking interface/socket in the initialization time with New(options ...Option).

This requires package modifications on container, service and daemon.
This requires code changes on importer, core proto, api/core, api/service and cmd/service.
(we can separate this as 3 PRs -or more- like feature/container, feature/service, feature/daemon)

There are some other packages that needs be refactored too like: database/services, execution etc. But I'll leave them for now and target these most important three: container, service and daemon.

Container

There is no major changes on container package except all the package level functions is now a method of Container type.

Please check this on going changes on container: https://github.com/mesg-foundation/core/tree/feature/container

Service

Issues

• 1- Consumers of service package directly uses database/services package to get a Service. This functionality should be provided by service package itself. database/services interactions should be made internally within service package. This is also good for separation of logic in the code and unit testing. (See new Manager API related to this)

• 2- Manually craft Service, Event, Task, Output, Parameter, Dependency types. I really don't like extending types generated by protocol buffers. It's not possible to add private fields to Service because of that. I need to extend Service type with some private fields to set the container client interface and other information needed by the new version of service API.

Package level functions

Some of the public/private APIs of service package:

• service.ParseYAML(data []byte) (Definition, error) > error will be filled validation errors if exists
• service.ParseDockerfile(data []byte) (Dockerfile, error) > error will be filled validation errors if exists
• service.NewManager(options ...Options) (*Manager, error)
• service.LevelDBOption(path string) Option
• service.ContainerDockerClientOption(client docker.CommonAPIClient) Option > used to mock container package's Docker Client.

Introduce Manager type

Some of the public/private APIs of Manager:

• Manager.GetService(id string) (*Service, error)
• Manager.CreateService(definition Definition) (*Service, error)
• Manager.CreateServiceFromYAML(data []byte) (*Service, error)
• Manager.DeleteService(id string) (*Service, error)
• Manager.ListServices(options ...ListServiceOption) ([]*Service, error)

Introduce Service type

Remove the generated Service type by protocol buffers and and create a new Service by hand.
Some of the public/private APIs of Service:

type Service struct {
        ID string
	Definition Definition
	manager *Manager
        container *container.Container
	...
}

• Service.ID
• Service.Definition
• Service.Start() error > call Service.stopDependencies() on partial running.
• Service.Stop() error
• Service.Status() (container.StatusType, error)
• Service.Dependencies() []*Dependency
• Service.EventSubscriptionChannel() string
• Service.ResultSubscriptionChannel() string
• Service.GetEvent(key string) (*Event, error) > error can be nil or EventNotFoundError
• Service.GetTask(key string) (*Task error)

Introduce Event type

Remove the generated Event type by protocol buffers and and create a new Event by hand. Some of the public/private APIs of Event:

• Event.Key
• Event.ValidateParametersSchema(data interface{}) error > error can be nilor InvalidEventDataError

Introduce Task type

Remove the generated Task type by protocol buffers and and create a new Task by hand. Some of the public/private APIs of Task:

• Task.Key
• Task.ValidateParametersSchema(data interface{}) error
• Task.GetOutput(key string) (*Output, error)

Introduce Output type

Remove the generated Output type by protocol buffers and and create a new Output by hand. Some of the public/private APIs of Output:

• Output.Key
• Output.ValidateSchema(data interface{}) error

Introduce Dependency type

Remove the generated Dependency type by protocol buffers and and create a new Dependency by hand. Some of the public/private APIs of Dependency:

• Dependency.Name
• Dependency.Logs() (io.ReadCloser, error)
• Dependency.start(networkID string) (containerServiceID string, err error)
• Dependency.stop() error
• Dependency.status() (container.StatusType, error)

Introduce Definition type

type Definition struct {
	Name          string                
	Description   string               
	Tasks         map[string]*Task      
	Events        map[string]*Event      
	Dependencies  map[string]*Dependency 
	Configuration *Dependency            
	Repository    string                
}

Introduce Parameter type

Remove the generated Parameter type by protocol buffers and and create a new Parameter by hand.

Introduce Dockerfile type

Dockerfile holds information about parsed Dockerfile of service.

Updates on importer, core proto, api/core, api/service and cmd/service

• Do not make direct access to importer from cmd anymore. CoreClient.DeployService()'s CoreClient.DeployServiceRequest will get a YAML (in bytes) field and Service field will be removed. Parsing/Validation of service file should be done with service.ParseYAML(data []byte) (Definition, error) and Manager.CreateServiceFromYAML(data []byte) (*Service, error)
• merge service.proto into api.proto and update api/core accordingly to migrate Definition data gathered from calls to Manager.GetService() and Manager.ListServices() to gRPC response types.
• Update Server type of api/core and api/service by adding a service(*service.Service) field. This way it's possible to mock docker API calls initialized by service package inside api/core and api/service. Useful for unit testing of api/core and api/service.
• don't do event, task, task output validations within execute or event packages. e.g. event.Create(&service, request.EventKey, data). Instead we should be doing this in the body of gRPC handler func by using validation methods from the updated API of service package described above. Because we need to be sure to validate event, task, output before calling the methods of execute or event packages for the sake of more organized code logic.
• use service.ParseDockerfile(bytes) and service.ParseYAML(bytes) for validation command.

Daemon

Package level functions

Some of the public/private APIs of daemon package:

• daemon.Namespace() []string
• daemon.New(options ...Options) (*Daemon, error)
• daemon.ContainerDockerClientOption(client docker.CommonAPIClient) Option > used to mock container package's Docker Client.

Introduce Daemon type

Some of the public/private APIs of Daemon:

• Daemon.Start() (containerID string, err error)
• Daemon.Stop() error
• Daemon.Status() (container.StatusType, error)

Notes

• dockertest will be used to mock docker related API calls while unit testing container, service, daemon, api/core and api/service packages.
• we should first complete feature/container(refactor and unit test container) PR and then start refactoring feature/service and feature/daemon because these two uses functionalities from container.

[NicolasMahe]

2- Manually craft Service, Event, Task, Output, Parameter, Dependency types. I really don't like extending types generated by protocol buffers. It's not possible to add private fields to Service because of that. I need to extend Service type with some private fields to set the container client

What about inherence of the generated types?
How do you do to not use the generated files? We could implement the 'proto' interface but i'm not sure it's enough. And also, I really like that we use the .proto file as source of truth with an easy way to apply modification of the .proto file to our codebase.

// Message is implemented by generated protocol buffer messages.
type Message interface {
	Reset()
	String() string
	ProtoMessage()
}

---

I'm really agree with:

Do not make direct access to importer from cmd anymore. CoreClient.DeployService()'s CoreClient.DeployServiceRequest will get a YAML (in bytes) field and Service field will be removed. Parsing/Validation of service file should be done with service.ParseYAML(data []byte) (Definition, error) and Manager.CreateServiceFromYAML(data []byte) (*Service, error)

It's kinda related to https://github.com/mesg-foundation/core/issues/191

---

I also totally agree with:

don't do event, task, task output validations within execute or event packages. e.g. event.Create(&service, request.EventKey, data). Instead we should be doing this in the body of gRPC handler func by using validation methods from the updated API of service package described above. Because we need to be sure to validate event, task, output before calling the methods of execute or event packages for the sake of more organized code logic.

---

Also totally agree on adding getters to access properly and mainly return typed errors when dealing with maps.

Service.GetEvent(key string) (*Event, error) > error can be nil or EventNotFoundError

---

I like the concept of Service Manager but I don't like the name, it's too generic. But anyway, I don't have any better idea for now..
Related to #349

---

What is your suggestion for the folder structure of those new type? Still inside a big Service package?
As the new type will not expose anything on the package API, it could be enough.

[krhubert]

What about inherence of the generated types?

+1, we should use generated type, I don't see the point to implement those struct/interface again.

Consumers of service package directly uses database/services package to get a Service. This functionality should be provided by service package itself.

I think this should stay in /database pacakge simply. Like in webservers you have server part and database(ORM) part. They interact at some point. Even more, the database can be not aware of service.Service type and return raw bytes (and converting from raw to struct can be done by some converter interface. But I'm against merging database into service, I'd rather keep them separate.

service.ParseDockerfile(data []byte) (Dockerfile, error) > error will be filled validation errors if exists

Why keep parse Dockerfile logic in service - it should be a separate package (and based on dockerfile/parser). service should get parsed Dockerfile.

service.ParseYAML(data []byte) (Definition, error) > error will be filled validation errors if exists.

Same here, service doesn't need knowledge about config file (what format etc was used, it only needs the config struct)

service.LevelDBOption(path string) Option

It's related to database option (also the name of database inside public API should be replaced with a generic name - someone doesn't need to know what database is used internally)

Introduce Manager type

The Manger/ServiceManager type is must have 👍

Manager.CreateServiceFromYAML(data []byte) (*Service, error)

Same as above - Manager doesn't need to know how to how config file will look like.

Introduce Service type

type Service struct {
	manager *Manager
}

Why there is a link between service and manager? The manager is responsible for handling services, not the other way.

Introduce Event/Taks/.... type

It's about implementing all the types, where I agree with @NicolasMahe - we should be used the generated files.

[antho1404]

I agree with the previous feedbacks.

We should keep the generated files after I see your point to add some extra data that proto will not know and not generate so maybe we can do some kind of proxy on them like:

type ServiceDefinition struct {
  proto *Service
}

This might be a solution to keep the best of both worlds but I'm a bit afraid that it will make the code way more complicated and maybe it's not worth it.

---

For the database I agree with @krhubert that the service and the database should stay separated as a package level after I think that it can be a package from service like /service/database and the service package can expose some api like service.Get(id string) or (s *Service) Save() that will just delegate to the database package (maybe these function could have the config for the database in optional parameters).

It kind of make sense to have all "serialization" in a "database" package but at the same time it's really nice to have it per model that we want to serialize. In this case we can really have different way of serialization, services on leveldb, executions on redis...

---

Can you explain more the dockerfile type, I don't see the point of this one and where it might be needed because we actually don't parse the dockerfile, we just check its presence

[ilgooz]

1- @NicolasMahe

What about inherence of the generated types?
How do you do to not use the generated files? We could implement the 'proto' interface but i'm not sure it's enough. And also, I really like that we use the .proto file as source of truth with an easy way to apply modification of the .proto file to our codebase.

I strongly disagree.

The thing is it doesn't make sense for me to use gRPC related code, structs or generated proto files in service package. gRPC is just a network layer protocol that exposes core's features to network.

We should distinctly separate core's business logic and network layer from each other and only deal with gRPC related code in the api package.

We should simply think of it like this: Service package exposes fundamental features of core and api package responsible to share those features with the network over gRPC.

Writing less code by is not always good. And I don't want to use weird names while creating an another Service type just because generated go code also use it.

2- @krhubert
I think this should stay in /database pacakge simply. Like in webservers you have server part and database(ORM) part. They interact at some point. Even more, the database can be not aware of service.Service type and return raw bytes (and converting from raw to struct can be done by some converter interface. But I'm against merging database into service, I'd rather keep them separate.

I totally agree with this. We should have a separate database like we do now but call its service set/get/delete/update methods within the service package itself not from the outside.

e.g. remove this this and move to implementation of Manager.CreateServiceFromYAML(data []byte).

3- @krhubert
Why keep parse Dockerfile logic in service - it should be a separate package (and based on dockerfile/parser). service should get parsed Dockerfile.

Dockerfiles totally relevant with services because they describe how a service should run. But after second check I see that we don't actually parse the Dockerfile, instead we check if it's exsits or not.
Maybe we add more validations about it in future. Either case we should should update:

• service.ParseDockerfile(data []byte) (Dockerfile, error) > error will be filled validation errors if exists
• AS service.ValidateDockerfile(data []byte) error > error will be filled validation errors if exists

Not relevant with this but also update:

• service.ParseYAML(data []byte) (Definition, error) > error will be filled validation errors if exists
• AS service.ValidateYAML(data []byte) error > error will be filled validation errors if exists

4- @krhubert
Same here, service doesn't need knowledge about config file (what format etc was used, it only needs the config struct)

I don't mind having a Manager.CreateServiceFromYAML(data []byte) (*Service, error) for creating services because YAML is a fundamental part of describing MESG services.

• We can add more parser when needed in future. Maybe under service/parser.
• e.g. Add a YAML parser: ParseYAML(data []byte) Definition
• And use it like: Manager.CreateService(ParseYAML(data)) (*Service, error)

5- @krhubert
It's related to database option (also the name of database inside public API should be replaced with a generic name - someone doesn't need to know what database is used internally)

Doesn't makes much sense to me because every database has their own kind of configuration that we need to pass in the initialization time. e.g. if we use Redis we have to specify a network address alongside database name. So using a special Option for the database we're using makes more sense to me.

6- @krhubert
Why there is a link between service and manager? The manager is responsible for handling services, not the other way.

We may need to get a pointer to database/service and *Container instance to access from Service type.

[ilgooz]

I think this was recommended by @NicolasMahe too somewhere, we should never use core's packages/methods from cmd package. Instead we can add some more gRPC methods as needed for the job and make cli only communicate with core over network.

[ilgooz]

as a part of #337 (comment) we should not be calling this in cmd. Instead compress the whole folder in the path and send to core within core.DeployServiceRequest. All validations, building container image and so on should be handled within daemon and if it's successful core. DeployService() will reply with a service if not with errors/validation errors.

[ilgooz]

A note:

cmd package shouldn't run any business logic of mesg-core/daemon. It has one job, communicate with mesg-core's gRPC API, nothing more. We should thread cmd as a simple client. Think about this, maybe we're going to create more clients that communicates with mesg-core therefore, we don't want to duplicate logic between client and have an absolute separation with server/daemon/mesg-core. Like we can make a web UI for creating services and devs can upload their service folders to deploy and start, stop their services.

[ilgooz]

We can also wrap codes of each gRPC handler's body in the api package and seperate them to their own methods under another package, this way business logic in the handlers will become agnostic and will no need to be aware of gRPC handlers in the high level.

for ex:
codes in the body of ExecuteTask() can be moved to a such method that expect its own parameters and Options. And we call that method by passing its expected arguments and Options within ExecuteTask() with the values read from request * ExecuteTaskRequest

// ExecuteTask executes a task for a given service.
func (s *Server) ExecuteTask(ctx context.Context, request *ExecuteTaskRequest) (*ExecuteTaskReply, error) {
	reply := &ExecuteTaskReply{}
	// err can be validation errors, ServiceManager.GetService() errors etc.
	execution, err := s.mesg.ExecuteTask(request.ServiceID, request.TaskKey, request.InputData, options...)
	if err != nil {
		return reply, err
	}
	reply.ExecutionID = execution.ID
	return reply, nil
}

[ilgooz]

lets also consider following method to create services from a tar archive.
Manager.CreateServiceFromTar(r io.Reader) (*Service, error) this reads a tar file that contains service program, Dockerfile and a mesg.yaml. error can be validation errors or any kind of other errors produced while creating a service.