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

Feature/tag executions #362

Merged
merged 30 commits into from Aug 19, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f60f431
add tags to executions
antho1404 Aug 13, 2018
6ea2908
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 13, 2018
44aed51
fix failing tests
antho1404 Aug 13, 2018
372df7a
add tag filters to listen result request api
antho1404 Aug 13, 2018
d0e18e7
possibility to subscribe to results with specific tags
antho1404 Aug 14, 2018
f7b4c87
update changelog
antho1404 Aug 14, 2018
0f79d9c
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 14, 2018
d70e67a
add tags in the result or execution
antho1404 Aug 14, 2018
1e70f7c
use tags to filter results on cmd execute command
antho1404 Aug 14, 2018
15f8c10
fix tests
antho1404 Aug 14, 2018
12d3d3c
add tests for execution command
antho1404 Aug 14, 2018
9cf0d6c
add more tests
antho1404 Aug 14, 2018
807647b
update documentation for tags
antho1404 Aug 14, 2018
31445f2
rename subscribed functions
antho1404 Aug 14, 2018
bf23d88
pluralize tagFilters
antho1404 Aug 14, 2018
5f98a3f
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 14, 2018
545f806
add optional parameter
antho1404 Aug 15, 2018
f29309c
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 15, 2018
f1fc1ff
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 15, 2018
c7ba093
rename tags to executionTags
antho1404 Aug 15, 2018
e636606
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 16, 2018
635662b
Merge branch 'dev' into feature/tag-executions
antho1404 Aug 17, 2018
8fa1c71
simplify isSubscribedToTags function
antho1404 Aug 17, 2018
0937afe
remove empty line
antho1404 Aug 17, 2018
48fe5ac
fix typo
antho1404 Aug 17, 2018
d5477e7
changelog typo
antho1404 Aug 17, 2018
7117239
type documentation
antho1404 Aug 17, 2018
4d6b785
use uuid instead of time to monitor a specific execution
antho1404 Aug 17, 2018
52009d5
typo again
antho1404 Aug 17, 2018
4934408
refactor subscribe method
antho1404 Aug 18, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -15,6 +15,8 @@
- (#316) Delete service when stoping the `service dev` command to avoid to keep all the versions of the services.
- (#317) Add errors when trying to execute a service that is not running (nothing was happening before)
- (#344) Add `service execute --data` flag to pass arguments as key=value.
- (#362) Add `tags` list parameter for execution in order to be able to categorize and/or track a specific task execution.
- (#362) Add possibility to listen results with specific tag(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/to listen results with specific tag(s)/to listen to results with the specific tag(s)/


#### Removed
- (#303) Deprecate command `service test` in favor of `service dev` and `service execute`
Expand Down
133 changes: 81 additions & 52 deletions api/core/api.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 19 additions & 13 deletions api/core/api.proto
Expand Up @@ -79,13 +79,15 @@ message EventData {
// {
// "serviceID": "__SERVICE_ID__",
// "taskFilter": "__TASK_KEY_TO_MATCH__",
// "outputFilter": "__OUTPUT_KEY_TO_MATCH__"
// "outputFilter": "__OUTPUT_KEY_TO_MATCH__",
// "tagFilters": ["tagX"]
// }
// ```
message ListenResultRequest {
string serviceID = 1; // The Service ID. Generated when using the [`DeployService` API](#deployservice).
string taskFilter = 2; // __Optional.__ The task's key to filter. The task must match this key. The default is `*` which matches any task.
string outputFilter = 3; // __Optional.__ The output's key from the task to filter. The task must return this output's key. The default is `*` which matches any output.
string serviceID = 1; // The Service ID. Generated when using the [`DeployService` API](#deployservice).
string taskFilter = 2; // __Optional.__ The task's key to filter. The task must match this key. The default is `*` which matches any task.
string outputFilter = 3; // __Optional.__ The output's key from the task to filter. The task must return this output's key. The default is `*` which matches any output.
repeated string tagFilters = 4; // __Optional.__ List of tags to filter. This is a "match all" list. All tags in parameters should be included in the execution to match.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add "The" at the beginning of the comment to look the same as other comments

}

// The data received from the stream of the `ListenResult` API.
Expand All @@ -97,14 +99,16 @@ message ListenResultRequest {
// "executionID": "__EXECUTION_ID__",
// "taskKey": "__TASK_KEY__",
// "outputKey": "__OUTPUT_KEY__",
// "outputData": "{\"foo\":\"bar\"}"
// "outputData": "{\"foo\":\"bar\"}",
// "tags": ["executionX", "test"]
// }
// ```
message ResultData {
string executionID = 1; // The unique identifier of the execution.
string taskKey = 2; // The key of the executed task.
string outputKey = 3; // The output's key from the returned task.
string outputData = 4; // The output's data from the returned task, encoded in JSON.
string executionID = 1; // The unique identifier of the execution.
string taskKey = 2; // The key of the executed task.
string outputKey = 3; // The output's key from the returned task.
string outputData = 4; // The output's data from the returned task, encoded in JSON.
repeated string tags = 5; // The list of tags associated to the execution
}

// The request's data for the `ExecuteTask` API.
Expand All @@ -114,13 +118,15 @@ message ResultData {
// {
// "serviceID": "__SERVICE_ID__",
// "taskKey": "__TASK_KEY__",
// "inputData": "{\"foo\":\"bar\"}"
// "inputData": "{\"foo\":\"bar\"}",
// "tags": ["executionX", "test"]
// }
// ```
message ExecuteTaskRequest {
string serviceID = 1; // The Service ID. Generated when using the [`DeployService` API](#deployservice).
string taskKey = 2; // The task's key to execute.
string inputData = 3; // The inputs of the task to execute, encoded in JSON.
string serviceID = 1; // The Service ID. Generated when using the [`DeployService` API](#deployservice).
string taskKey = 2; // The task's key to execute.
string inputData = 3; // The inputs of the task to execute, encoded in JSON.
repeated string tags = 4; // The list of tags to associate to the execution
Copy link
Member

Choose a reason for hiding this comment

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

My argument to name it executionTags is to be as explicit as possible without needing anymore context.
Also, we name everything by using 2 words. So let's continue this way.

That's what we did for all other parameters. Eg:

https://github.com/mesg-foundation/core/blob/5f98a3f23a1653664fbf8e3a61bbbb1027cd378d/api/core/api.proto#L70-L73

https://github.com/mesg-foundation/core/blob/5f98a3f23a1653664fbf8e3a61bbbb1027cd378d/api/service/api.proto#L39-L43

Copy link
Member Author

Choose a reason for hiding this comment

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

for the rest of the conversation
https://github.com/mesg-foundation/core/pull/362/files/12d3d3cba19155ab54cb58118d3b4809656957e6#r209819975

I would love to have other feedbacks on that @ilgooz @krhubert ?

Naming problem again :)

For the naming of the tag parameter sent when executing a task or listening for the result of a task.

  • tags (simple, related to the context, can be reused in any other kind of resources to add metadata)
  • executionTags (strongly coupled to the execution, gives a lot of details without knowing the context, we often have 2 words attribute)

What do you think ?

}

// The reply's data of the `ExecuteTask` API.
Expand Down
6 changes: 3 additions & 3 deletions api/core/execute.go
Expand Up @@ -22,7 +22,7 @@ func (s *Server) ExecuteTask(ctx context.Context, request *ExecuteTaskRequest) (
if err := checkServiceStatus(&srv); err != nil {
return nil, err
}
executionID, err := execute(&srv, request.TaskKey, inputs)
executionID, err := execute(&srv, request.TaskKey, inputs, request.Tags)
return &ExecuteTaskReply{
ExecutionID: executionID,
}, err
Expand All @@ -45,8 +45,8 @@ func getData(request *ExecuteTaskRequest) (map[string]interface{}, error) {
return inputs, err
}

func execute(srv *service.Service, key string, inputs map[string]interface{}) (executionID string, err error) {
exc, err := execution.Create(srv, key, inputs)
func execute(srv *service.Service, key string, inputs map[string]interface{}, tags []string) (executionID string, err error) {
exc, err := execution.Create(srv, key, inputs, tags)
if err != nil {
return "", err
}
Expand Down
4 changes: 2 additions & 2 deletions api/core/execute_test.go
Expand Up @@ -126,14 +126,14 @@ func TestExecuteFunc(t *testing.T) {
"test": {},
},
}
id, err := execute(srv, "test", map[string]interface{}{})
id, err := execute(srv, "test", map[string]interface{}{}, []string{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can tags be nil? Everywhere there is an empty string slice, but I wonder if it's ok to use nil. I think it could be. (Maybe same for inputs?)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes nil is fine too. Do you want me to replace with nil instead

assert.Nil(t, err)
assert.NotNil(t, id)
}

func TestExecuteFuncInvalidTaskName(t *testing.T) {
srv := &service.Service{}
_, err := execute(srv, "test", map[string]interface{}{})
_, err := execute(srv, "test", map[string]interface{}{}, []string{})
assert.NotNil(t, err)
}

Expand Down
19 changes: 16 additions & 3 deletions api/core/listen_result.go
Expand Up @@ -32,13 +32,14 @@ func (s *Server) ListenResult(request *ListenResultRequest, stream Core_ListenRe
return ctx.Err()
case data := <-subscription:
execution := data.(*execution.Execution)
if isSubscribedTask(request, execution) && isSubscribedOutput(request, execution) {
if isSubscribedToTags(request, execution) && isSubscribedToTask(request, execution) && isSubscribedToOutput(request, execution) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to

if isSubscribedToTags(request, execution) && 
   isSubscribedToTask(request, execution) && 
   isSubscribedToOutput(request, execution) {
}

outputs, _ := json.Marshal(execution.OutputData)
Copy link
Contributor

Choose a reason for hiding this comment

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

Always check for err for even for json.Marshal (especially if there is an interface{} type as value)

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, it's present in other places too, I created a separated issue for that #374

if err := stream.Send(&ResultData{
ExecutionID: execution.ID,
TaskKey: execution.Task,
OutputKey: execution.Output,
OutputData: string(outputs),
Tags: execution.Tags,
}); err != nil {
return err
}
Expand Down Expand Up @@ -79,10 +80,22 @@ func validateOutputKey(service *service.Service, taskKey string, outputFilter st
return nil
}

func isSubscribedTask(request *ListenResultRequest, e *execution.Execution) bool {
func isSubscribedToTask(request *ListenResultRequest, e *execution.Execution) bool {
return array.IncludedIn([]string{"", "*", e.Task}, request.TaskFilter)
}

func isSubscribedOutput(request *ListenResultRequest, e *execution.Execution) bool {
func isSubscribedToOutput(request *ListenResultRequest, e *execution.Execution) bool {
return array.IncludedIn([]string{"", "*", e.Output}, request.OutputFilter)
}

func isSubscribedToTags(request *ListenResultRequest, e *execution.Execution) bool {
if len(request.TagFilters) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Len check not needed? if len == 0 the for won't be executed and funcs return true anyway.

return true
}
for _, tag := range request.TagFilters {
if !array.IncludedIn(e.Tags, tag) {
return false
}
}
return true
}