-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
f725270
to
bdab17d
Compare
…ve service.proto to interface/grpc/core * pb files updated by running the latest version of generator
bdab17d
to
107f1a1
Compare
There was a problem hiding this 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 ?
cmd/service/delete.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
interface/grpc/core/core.go
Outdated
return sv | ||
} | ||
|
||
func toGRPCParameters(params map[string]*service.Parameter) map[string]*Parameter { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
interface/grpc/core/service.proto
Outdated
|
||
// 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
service/parameter.go
Outdated
Type string `hash:"name:3" yaml:"type"` | ||
|
||
// Optional indicates if parameter is optional. | ||
Optional bool `hash:"name:4" yaml:"optiona"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here: optiona
I notice that the command logs doesn't work anymore. But since, you put a todo, and my PR on the config package will report error, it should be fix very soon. |
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.