Refactoring list

Hi,

I sketched a draft of possible refactors. I know there are quite a few, but I’m happy to solve them.
If you could divide them into 3 category

a) totally agree
b) nor agree nor disagree
c) totally disagree

It would be awesome. Such categorization tells which refactor everybody is ok with, then talk about bullet points from category b and the last one from c.

Please give a list of them in format, starting from a which is most important one

a: comamnds.1, general.6 etc…
b: …
c: …

As for me, I see all of them as cat. A (otherwise I wouldn’t write them). If something is unclear let me know - I will clarify my points. If you fill something is missing let me know, then I will add it to this list.

I aware of fact that there are some easy tasks to do as well as more complex one that for sure needs futher discussion, but it’s a starting point.

General

  1. (a) keep task inputData naming consistent across packages (now is inputs/tasksInputs/inputData) - also check if other namings are consistent.
  2. move mocks from mocks directory to package level
  3. clean/remove for simplicity StatusType - for now there are 4 StatusTypes in 4 different places just to pass it to a command line (maybe we can replace it with github.com/docker/docker/pkg/streamformatter or github.com/docker/docker/pkg/progress)
  4. rename all folder with test data to testdata
  5. change integration_test.go to -short flag (as it’s could be more controlled from tests - for example, all test will be in one file)
  6. (a) do not call config. Global inside packages - pass config
  7. move interface/cli/main.go and core/main.go to
  • cmd/core and cmd/cli (standard in go)
  • ./main.go (core) and ./cli/main.go (cli)

api

  1. change api name for sdk, or even better api it is just our server in fact (same like rest servers)
    we should keep server implementation there. Remind yourself if you have written http server did you implement the logic there or keep out and the server was just to set up routes? here we don’t have even routes so almost all grpc/core|service methods are oneliners
  2. move Delete method body to Delete Service and so on…

commands

  1. (a) move Stop logic in core_provider to container/daemon - ideally api should be an interface and it should be passed to commands
  2. remove as much logic as possible from providers

container

  1. change package name from container to something more general (maybe docker), because here core handle containers, but also services, swarm and more
  2. (a) change dockertest to testify mocks
  3. replace func presenceHandling with return !docker.IsErrNotFound(err), err
  4. remove client_test.go as it’s probably outdated
  5. use github.com/docker/cli/cli/command/image/build.ReadDockerignore
  6. see if we can use something useful from docker/cli

config

  1. remove loading config with sync and pass it directly (do we still need to load config many times?)
  2. combine Service And Execution Relative Path into one (user don’t need to know internal database layout)

dameon (done)

  1. (c) rename Deamon as it’s more associated with running something in the background (not in the container)
  2. (done) (a) remove init function
  3. (done) (a) make Daemon newable
  4. (done) (a) combine all files into one (almost all functions are oneliner)
  5. (done) (a) remove Test Logs, Stop, Status and Start (left only test for serviceSpec) as all of them wrappers and they are tested in service/container packages.

database

  1. replace leveldb with badger (performance) (https://github.com/mesg-foundation/core/issues/565)
  2. remove mocks (as long as we don’t use some mocks (or code, in general, we should avoid putting it into repo))

event

  1. get rid of it - there is only one method and it’s just for publishing data - it should be in different place

execution

  1. keep ID as byte (fmt.Sprintf("%x") is a human-readable format, not for id store it should stay byte)

interface

  1. grpc/server , grpc/core, grpc/client and grpc/service so these are similar. I talked about the possibilites to refactor while describing api. Also, the destination of this packages should be clear. Either we keep:
  • full validation, data mapping, logic connected with api here
  • basic validation, data mapping (no logic)
  • data mapping
    Now there are some methods that are simply oneliner around api and there are some like core/deploy.go where a lot of stuff is going on
  1. interface/grpc/client/workflow.go - strings.Compare(wf.OnEvent.Name, “*”) == 0 - do not use strings.Compare -> use ==
  2. move tests like TestDeleteService to api package (same for grpc/servce)

pubsub

  1. make pubsub newable

service

  1. move service-test directory to service package
  2. importer - replace read dockerfile with validation (github.com/moby/buildkit/frontend/dockerfile/parser.Parse)
  3. importer - simplify reading/validation - there should be one method - Parse which returns definition and error (all validation and other stuff should be done while reading from a path)
  4. so this package contains a lot of stuff that should be splitted - it contains
  • service validation - if mesg.yml dockerfile etc are ok
  • service container managing - start, stop, logs etc
  • service dependencies manager - start-stop handling etc
  • task/inputs/outputs definition
    all of above are in one package (expect validations) and all of them are part of one big Service struct. I think there should be struct for deploying/manage dependencies, containers etc…
  1. rename service.FromService to service.FromDefinition (https://github.com/mesg-foundation/core/issues/436)
  2. ! Change Ouptus type to map[string*Parametr the same as input (what was the reason to define output in diffrent way then input?)
type Task struct {
  // Inputs are the definition of the execution inputs of task.
  Inputs map[string]*Parameter `yaml:"inputs"`
  
  // Outputs are the definition of the execution results of task.
  Outputs map[string]*Output `yaml:"outputs"`
}
  1. we should completely change the way how to serialise/deserialise service to keep it’s definition in db.
  2. Change Status method to ServiceStatus

utils

  1. move clierror to commands package

others

  1. change repository name to mesg-foundation/mesg
  2. change mesg-core to mesg-cli or mesgcli as it is actually cli (https://github.com/mesg-foundation/core/issues/365)
  3. include gometalinter to pipeline build, as we have very few errors (most of them are misspelled)
➜  core git:(refactor/todo) ✗ gometalinter --tests --vendor --disable-all --enable=golint --enable=goimports --enable=vetshadow --enable=misspell --enable=vet --min-confidence=1 ./...
container/dockertest/client.go:53:46:warning: "paramaters" is a misspelling of "parameters" (misspell)
utils/pretty/pretty.go:255:18:warning: "destroyes" is a misspelling of "destroys" (misspell)
utils/pretty/pretty.go:374:18:warning: "destroyes" is a misspelling of "destroys" (misspell)
service/importer/assets/schema.go:1::warning: file is not goimported (goimports)
container/container_test.go:131::error: github.com/mesg-foundation/core/vendor/github.com/docker/docker/api/types.ServiceInspectOptions composite literal uses unkeyed fields (vet)
commands/provider/service_provider.go:131::warning: declaration of "err" shadows declaration at commands/provider/service_provider.go:129 (vetshadow)
commands/service_dev.go:80::warning: declaration of "err" shadows declaration at commands/service_dev.go:65 (vetshadow)
service/start.go:22::warning: declaration of "err" shadows declaration at service/start.go:15 (vetshadow)
service/dependency_test.go:54::warning: declaration of "err" shadows declaration at service/dependency_test.go:45 (vetshadow)
x/xos/os_test.go:59::warning: declaration of "err" shadows declaration at x/xos/os_test.go:51 (vetshadow)
x/xos/os_test.go:63::warning: declaration of "err" shadows declaration at x/xos/os_test.go:51 (vetshadow)
main.go:10:7:warning: exported const Dockerfile should have comment or be unexported (golint)
commands/provider/assets/readme_template.go:1:1:warning: package comment should be of the form "Package assets ..." (golint)
daemon/init.go:1:1:warning: package comment should be of the form "Package daemon ..." (golint)
1 Like

a

General.1
General.5: yes!
General.6
General.7: cmd/core and cmd/cli (standard in go)
api.1
api.2
commands.1
commands.2
container.2
daemon.2
daemon.3
event.1: agree. it should be in the future sdk package

b

General.3
General.4
container.3: i don’t understand
container.4
container.5
container.6
config.1: what do you mean? agree that we don’t need to load it many time
daemon.4
execution.1 i don’t understand

c

General.2: so the mocks should be in the same package as the interface? I prefer to separate the interface, implementation and mock in different packages.
container.1: I think we should move a lot more logic from the service package to the container package, and more all docker related implementation to a docker package. We should have a container package that can start a service (with its dependencies and networks) and is not stuck with the logic of docker. We want to be able to switch from docker to something else without breaking the core.
config.2: this is not a user config. Core.Path is the user config. We mix both for now. Any suggestion on how to separate user config to core config? We should keep config constant in one file.
daemon.1: It’s running in the background… in docker.
database.1: it should be moved to a system service
database.2: we will use api and system services mocks once move to a system service


@krhubert lots of good suggestion :wink: But I didn’t understand some. I will continue tomorrow. I stop at interface.

a

  • general-1
  • general-6
  • commands-1
  • container-2
  • container-3
  • container-4: should add equivalent if needed
  • daemon-2 & daemon-3
  • daemon-4
  • pubsub-1
  • service-2
  • service-3
  • utils-1
  • others-3

b

  • general-2: Having generated code in a separate pkg feels more clean to me. Code base and GoDoc stays much more readable.
  • general-3: I don’t understand what do you suggest to solve it.
  • general-4: Can you provide some links to show current ones?
  • general-7: I suggest following to keep cli programs consistent, in the same place:
    • client cli: interface/cli/mesg-core/main.go > this is also go get’abble. binary name will be installed as mesg-core.
    • daemon cli: interface/cli/core/main.go
  • api-1: I don’t understand what do you suggest and the problem.
  • api-2: As long as wee keep the separation in func names. Because not all APIs are consist of only with one func.
  • commands-2: Can you provide examples with links?
  • container-1: There is an ongoing discussion about this. service & container pkg’s structure may change a lot in future when we start adopting kubernetes and others.
  • container-5: I don’t understand, need more info.
  • container-6: For what?
  • config-1: What do you mean by sync, need more info.
  • config-2
  • daemon-1: Maybe as core or core/docker. In future we may use OS’ service managers: core/systemd.
  • database-1: I don’t have my research.
  • event-1: Where it should be in?
  • execution-1: I don’t understand what is the problem with current string type.
  • interface-1: I don’t understand the problem and what do you suggest, need more details. grpc/client should be completely removed, it’s not used.
  • service-1: It is used by other packages too. So I’m not sure if should do this change. If we do we can put it under service/testdata.
  • service-5: Changing only func name is not enough we should completely change the way how to serialise/deserialise service to keep it’s definition in db. Saving Service struct directly in db is error-prone because upgrading it can be forgotten.
  • others-1: Maybe.
  • others-2: Maybe to mesgf (mesg-foundation)

c

  • general-5: I think we can use -short flag but still separate integrations tests and unit tests in different places.
  • database-2: We discussed about this before and decided to have it.
  • interface-2: pkg should be completely removed.
  • interface-3: We should keep tests in both sides but mock api pkg while testing interface/* pkgs.
  • service-4: I don’t quite understand why and the problem.
  • service-6: What do you mean? They’ve different struct fields. Parameter should be used to define input data.
  • general-3: I don’t understand what do you suggest to solve it.

rpc DeployService (stream DeployServiceRequest) returns (stream DeployServiceReply) {}
I suggested to remove strem and return simply DeployServiceReplay because It will be enough to tell the users if deploy is ended with success or fail. Moreover if we want to keep this the StatusType is defiend 4 times:

api/deploy.go:type StatusType int
service/status.go:type StatusType uint
container/type.go:type StatusType uint
commands/provider/service_deployer.go:type StatusType int

And there is a lot of passing chanels etc just to print deploy status. There should be just one StatusType.

  • general-4: Can you provide some links to show current ones?

And some others, but this is is first coming to my mind

general-7: I suggest following to keep cli programs consistent, in the same place:

Yep, but first core is not cli, and keeping under root or cmd directories are more “go way”

  • api-1: I don’t understand what do you suggest and the problem.

So the api is, for example, Grpc API, or HTTP api etc… here we have the actual implementation which is used by grpc server. So it’s kind of api/kind of not api

  • api-2: As long as wee keep the separation in func names. Because not all APIs are consist of only with one func.

What do you mean by separation in fucs names? Of course each function will have it’s onw name

  • commands-2: Can you provide examples with links?

ServiceDeleteAll
ServiceLogs
ServiceListenResults
ServiceListenEvents


Stop

  • container-1: There is an ongoing discussion about this. service & container pkg’s structure may change a lot in future when we start adopting kubernetes and others.

Yes, but I’m talkin about refactoring and clean it a little bit.

  • container-5: I don’t understand, need more info.
  • container-6: For what?


dockerignoreFiles -> replace this one

Or

package main

import (
	"bytes"

	"github.com/moby/buildkit/frontend/dockerfile/instructions"
	"github.com/moby/buildkit/frontend/dockerfile/parser"
)

const Dockerfile = `
FROM ubuntu:18.04
XXX
`

func main() {
	r := bytes.NewBufferString(Dockerfile)

	dockerfile, err := parser.Parse(r)
	if err != nil {
		panic(err)
	}
	_, _, err = instructions.Parse(dockerfile.AST)
	if err != nil {
		panic(err)
	}

}

There are some packages to validate dockerfile (not only if this exists as we have now). I looked at docker code and there is a lot of cool stuff. I don’t know what else we can use form them, but there are 2 suggestions for now.

  • config-1: What do you mean by sync , need more info.

sync.Once - just to load config at the start of program (as we don’t use this feature)

  • config-2

Need more info

  • daemon-1: Maybe as core or core/docker . In future we may use OS’ service managers: core/systemd .

That’s true, I dont have package name right now, but if we don’t use this as daemon right now

  • database-1: I don’t have my research.

I hit the resach when I worked on tendermint

  • event-1: Where it should be in?
api/emit_event.go:	event.Publish()

There is exeacly one place where publish is called - so here we can replace it with pubsub.Publush (we are using it already in api package)

So in event package will be one struct - maybe it should be in api maybe somewhere else, but I think one package just for one struct is too much

  • execution-1: I don’t understand what is the problem with current string type.

There is no pbm with it, but handling byte is general way efficient format to work with (sending through network etc). The is a reason why sha1.Sum returns the byte, io.Reader/Writer reads/writes bytes etc…

  • interface-1: I don’t understand the problem and what do you suggest, need more details. grpc/client should be completely removed, it’s not used.

So you have:
https://github.com/mesg-foundation/core/blob/dev/interface/grpc/core/delete.go : wrapper aourd api
https://github.com/mesg-foundation/core/blob/dev/interface/grpc/core/list_services.go : doing some stuff after ListeServices to get thier status and then map values to grpc struct
https://github.com/mesg-foundation/core/blob/dev/interface/grpc/core/logs.go : get logs and them stream it
https://github.com/mesg-foundation/core/blob/dev/interface/grpc/core/execute.go : map data before Execute tasks
https://github.com/mesg-foundation/core/blob/dev/interface/grpc/core/deploy.go : etc …

So as you can se some functions are just wrappers, some doing more stuff (like getting the status of service) and some just map/stream data, so there is everything here, and the purpose of grpc package should be clear.

  • service-1: It is used by other packages too. So I’m not sure if should do this change. If we do we can put it under service/testdata .

We can changed paht in other packages too, or just split them (as I remember not all services are used by every test).

  • service-5: Changing only func name is not enough we should completely change the way how to serialise/deserialise service to keep it’s definition in db. Saving Service struct directly in db is error-prone because upgrading it can be forgotten.

Sure, It was just a start, I will add it to list as poitn servcie-7

  • general-5: I think we can use -short flag but still separate integrations tests and unit tests in different places.

So it it will be exacly the same as we have right now

  • database-2: We discussed about this before and decided to have it.

Yes, but I’m raising it agin, as this mocks is used nowhere and don’t see the point to keep it, just because we can

  • interface-2: pkg should be completely removed.

Which one?

  • interface-3: We should keep tests in both sides but mock api pkg while testing interface/* pkgs.

Yes, but for now we testing api functionalities in core (I mean we test wrapper around one api method) so we test api in fact.

  • service-4: I don’t quite understand why and the problem.

There is a lot of stuff in one place in one structure - as I wrote service package hendles:
container managing - start , stop etc
service and dependiencies managing
validations etc …

I jsut want to keep it clean so for examplem
move part about manaing container to container pkg
create struct for manage dependencies
etc …

  • service-6: What do you mean? They’ve different struct fields. Parameter should be used to define input data.

Yes, I’m asking what was the reason, why they are diffrent if they could be the same? You define inputs in mesg.yaml and then outputs are diffrent, but they both represent data types

Waiting for whole list and the resons for b

container.3 : i don’t understand

https://github.com/mesg-foundation/core/blob/fe98a412386c37fcbdb4b391c1bb4941110bc34f/container/container.go#L185 : simplify this funciton

config.1 : what do you mean? agree that we don’t need to load it many time

I mean to just load It once on start - so standard process load/init config in the func main() and the pass it. While doing so we don’t need Global function nor sync.Once

execution.1 i don’t understand

https://github.com/mesg-foundation/core/blob/a940f93a430cc778fe952a5e09a6468ec8443f0e/execution/execution.go#L59: here is kept as string, but should be bytes (look also on my replay to @ilgooz)

General.2 : so the mocks should be in the same package as the interface? I prefer to separate the interface, implementation and mock in different packages.

So it’s not “go way”. Like that you want to have eg. commands package where
commands/interface.go is the interface
commands/impl/ - is package for implementation
commands/mocks/ - is package for mocks

This is defenetly not from go, in go you should keep it in one palce. Right now there are two problems:

For example you imports mocks and create some package

  1. mocks.NewExecution() - you have to go to imports/definition and check where this is defiend as there (oh it’s executor from commands package. Compare to this commands.NewMockExecution() - you see everthing right away)
  2. if you need to import two diffrent mocks:
import (
mocksA ".../database/mocks"
mocksB ".../filesystem/mocks"
)
  1. https://rakyll.org/style-packages/ -> “In go, package names are not plural. This is surprising to programmers who came from other languages and are retaining an old habit of pluralizing names. Don’t name a package httputils, but httputil!”
  2. if we avoid prulars the package becomes mock and this is already taken by testify/mock (here mock is the purpose of package -> provide mocking)

In go good practice is to avoid named imports (so no imports should be named). At the early days of golang the pople started to name package as go-database, go-something which resulted in many alias imports (because you can’t use go-database.New etc…).

container.1 : I think we should move a lot more logic from the service package to the container package, and more all docker related implementation to a docker package. We should have a container package that can start a service (with its dependencies and networks) and is not stuck with the logic of docker. We want to be able to switch from docker to something else without breaking the core.

Yes, this is what I ment. See also other my responses - so why it is in C?

config.2 : this is not a user config. Core.Path is the user config. We mix both for now. Any suggestion on how to separate user config to core config? We should keep config constant in one file.

It is user config if we keep it under one structure and you can’t forbid people to use it. What I mean here is just core dosne’t need to have spereate database and systemservices config (they can create them in cone.path joins with core.name)

One suggest will be to keep core config under core: and the current core config under mesg: or something like that

daemon.1 : It’s running in the background… in docker.

background daemon != docker cointaer (both are running in the background but one is daemon and the other is container)

database.1 : it should be moved to a system service

It’s about replacing it , not move it - could you tell more?

database.2 : we will use api and system services mocks once move to a system service

Once we need it we will create it. By then a database will change a lot probably.

  • general-3: I don’t think removing a feature does make sense. If you’ve a solution thats great.
  • general-7: It’s a cli if it’s a main pkg. cli also a good name and I see that it’s widely used.
  • api-2: We talked about this a lot, even in the issues/PRs while refactoring. Each file in api pkg hosts an API method. And this method can be complex and it’s code may be needed to splitted to different smaller funcs. If we define these funs globally in the api pkg this is a bit messy to me. This is why we have structs to host these small funcs as their methods. This way, they’re private and cannot be acceded by other files. Func names from other files cannot conflict with each other and we can give short names to them without worrying.
  • commands-2: I still don’t understand why we need to reduce logic from here? Instead we should be writing tests for them. I think keeping commands smaller kinda makes more sense.
  • container-5: I agree that parsing Dockerfile can be useful in importer pkg but here for this bullet we’re talking about .dockerignore.
  • config-1: I think we somehow wanted more control on it and made it Newable because of that. Maybe to change its behavior for different environments like testing.
  • config-2: I think I’m more closer to keep one as it’s, each db different folders.
  • execution-1: I don’t mind both ways, it’s up to the database we use.
  • interface-1: Yes it should be just a wrapper. I don’t see any problem with it. gRPC pkgs should not have any logic related to Core.
  • interface-2: interface/grpc/client/
  • service-4: I don’t quite understand maybe you can create a detailed proposal for this one.

I lot of work has been done since this post. A huge majority of the proposed changes are either implemented or not relevant anymore.