Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service: use custom Service, Event, Task, Output, Parameter types #414

Merged
merged 8 commits into from Sep 4, 2018

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Aug 30, 2018

fundamental part of #337.

This is an intermediate PR for service refactoring. This PR only contains transition to new types. Auto-generated gRPC types moved under interface/grpc/core.

Next PRs will introduce new features and unit tests to service package and there'll be some refactoring on the code as well.

@ilgooz ilgooz added this to the v1.2.0 milestone Aug 30, 2018
@ilgooz ilgooz self-assigned this Aug 30, 2018
@ilgooz ilgooz added this to In progress in v1.2 via automation Aug 30, 2018
@ilgooz ilgooz force-pushed the feature/service-custom-types branch from f725270 to bdab17d Compare August 31, 2018 05:03
…ve service.proto to interface/grpc/core

* pb files updated by running the latest version of generator
@ilgooz ilgooz force-pushed the feature/service-custom-types branch from bdab17d to 107f1a1 Compare August 31, 2018 06:06
@ilgooz ilgooz requested review from krhubert, antho1404 and NicolasMahe and removed request for krhubert, antho1404 and NicolasMahe August 31, 2018 06:14
@ilgooz ilgooz moved this from In progress to Needs review in v1.2 Aug 31, 2018
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had this kind of proxy for the task, task.output and task.output.data.
Why not for task.inputs, events, dependencies... basically everywhere where we have map ?

@@ -43,7 +43,7 @@ func deleteHandler(cmd *cobra.Command, args []string) {
return
}
for _, service := range reply.Services {
args = append(args, service.Hash())
args = append(args, service.Id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why there is sometime Id and sometime ID ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID comes from our custom Service type. Id comes from gRPC Service type because it cannot convert id field described in the proto file to full-uppercase.

database/services/all.go Show resolved Hide resolved
return sv
}

func toGRPCParameters(params map[string]*service.Parameter) map[string]*Parameter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use toProtoParameter ? I think the data are related to protobuf and sent via grpc. Small detail and I'm fine with that it's just to think about it maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// This is the definition of a MESG Service.
message Service {
string id = 10; // Service's unique id hash.
string id = 10; // Service's unique id service hash.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use ID here and like that we only have ID in the codebase and not Id and ID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ilgooz ilgooz mentioned this pull request Sep 2, 2018
antho1404
antho1404 previously approved these changes Sep 2, 2018
Type string `hash:"name:3" yaml:"type"`

// Optional indicates if parameter is optional.
Optional bool `hash:"name:4" yaml:"optiona"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here: optiona

@NicolasMahe
Copy link
Member

NicolasMahe commented Sep 3, 2018

I notice that the command logs doesn't work anymore.
I think it's because of the use of Docker volume and of the following line that try to read the database from the cli:
https://github.com/mesg-foundation/core/pull/414/files#diff-2c4fa6ab22f8116fb149c75bbd787027R43

But since, you put a todo, and my PR on the config package will report error, it should be fix very soon.

@ilgooz ilgooz mentioned this pull request Sep 3, 2018
@antho1404 antho1404 merged commit 9531a5c into dev Sep 4, 2018
v1.2 automation moved this from Needs review to Done Sep 4, 2018
@antho1404 antho1404 deleted the feature/service-custom-types branch September 4, 2018 03:23
@ilgooz ilgooz restored the feature/service-custom-types branch September 4, 2018 03:45
@ilgooz ilgooz deleted the feature/service-custom-types branch September 4, 2018 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v1.2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants