Add data to the service execute with no file #344

krhubert · 2018-08-06T05:51:52Z

related to #256

NicolasMahe

What is xpflag? Can you provide some example on how to use it?

NicolasMahe · 2018-08-06T11:08:03Z

cmd/service/execute.go

+	if (data != "" && jsonStr != "") ||
+		(data != "" && jsonFile != "") ||
+		(jsonStr != "" && jsonFile != "") {
+		return "", errors.New("data, json and json-file flags are  mutually exclusive")


Can you write something more user friendly like: "You can use only of one of the following flags: 'data', 'json' and 'json-file'."

Sure, done.

As xpflag is eXtension to spf13/pflag that contains custom flags (implements pflag.Value interface) and I put it in different package.

antho1404 · 2018-08-06T11:18:34Z

This one will definitely need some update on the documentation and changelog and also I'm not really sure about it first we already have a json that reads a file and not it does something different. Also I don't see the point of having a json directly in the command (not in the file) and having a data.

krhubert · 2018-08-06T17:40:21Z

@antho1404 For sure, I've kept 3 options for further discussion (which of them should/shouldn't stay).
As for JSON in command, I find it frustrating when I wanted to execute some task I had to create a file with simple JSON, then edit the file in order to execute with different task data, etc...

It's nice to have json in cmd:

$ mesg-core service test --task execute --data '{"key":"1"}'
$ mesg-core service test --task execute --data '{"key":"2"}'
$ # do more

antho1404 · 2018-08-07T06:15:00Z

I was actually against json as parameter because if using double quotes it looks like
"{\"foo\":\"bar\"}" which is terrible to write but with single quotes it's way better. I'm just afraid for a user perspective they might do this mistake and spend hours to understand why their data are not good. Users might be junior developers so probably not that much comfortable with the terminal.

I like the data part but this will be interpreted as string and if the data needs to be boolean for example the core will reject the task so this is something to manage too

NicolasMahe

JSON with space doesn't work when passed:

service execute v1_120cc5caa2a7ccea96767d19929ad6fb --task transfer --json  '{"to": "0xadwwadawd", "value": 27, "contractAddress": "0xdawdwaw", "privateKey": "0xawdawawd"}'

I'm not sure what to do with JSON. It's quite terrible to use JSON in the terminal....
I'm happy remove the --json and keep the --data flag.

And as said by Anthony, data flag return only strings type:

./dev-cli service execute v1_120cc5caa2a7ccea96767d19929ad6fb --task transfer --data to=0xwadadawd,value=27,contractAddress=0xdwdwdwdw,privateKey=0xdwdwdwdw

⣻ Executing task transfer... rpc error: code = Unknown desc = Inputs of task 'transfer' are invalid. Value of 'value' is not a number

@krhubert Can you cast the type when using the flag data? You can use the API GetService to access the service's definition.

serviceReply, err := cli().GetService(context.Background(), &core.GetServiceRequest{
  ServiceID: serviceID,
})

NicolasMahe · 2018-08-07T09:04:53Z

cmd/service/execute.go

@@ -11,11 +13,13 @@ import (
 	"github.com/mesg-foundation/core/api/core"
 	"github.com/mesg-foundation/core/cmd/utils"
 	"github.com/mesg-foundation/core/service"
+	"github.com/mesg-foundation/core/utils/xpflag"
 	"github.com/spf13/cobra"
 	survey "gopkg.in/AlecAivazis/survey.v1"
 )

 var executions []*core.ResultData


executions is not used. (not related to this PR)

don't get it, what do you mean? executions wasn't touched

Yes I know, that's why I said "(not related to this PR)". But still, could you remove it?

NicolasMahe · 2018-08-07T09:28:16Z

@mesg-foundation/core What about asking the value of the task's inputs by questions if no data has been passed by flags? It's not the most efficient way to pass data, but for a quick test of a task it's super convenient! No need to know the inputs before executing the command.

krhubert · 2018-08-07T10:20:14Z

I don't like asking about the data. It makes harder for debuging (if someone run the cli in some script and it hangs waiting for user input). I prefer return non 0 value + message

NicolasMahe · 2018-08-07T10:22:24Z

We ask quite a lot of questions in the different command already.
We only ask when flags don't exist. So from a script, the dev will use flags.

NicolasMahe · 2018-08-07T10:55:56Z

To sum up the conversion on discord:

Revert --json flag to accept a JSON file
Keep flag --data. But be compatible with: --data key1=value1 --data key2=value2
Cast every value passed with the data flag using the type defined in the definition of the service

antho1404 · 2018-08-07T11:28:58Z

I would add also

update the documentation https://github.com/mesg-foundation/core/blob/fc75e9b86e16947a351f0202ea72a16c6216941f/docs/guide/service/test.md
fix codeclimate
update the changelog

NicolasMahe · 2018-08-08T04:27:31Z

service/cast.go

+func (s *Service) Cast(taskKey string, taskData map[string]string) (map[string]interface{}, error) {
+	task, ok := s.Tasks[taskKey]
+	if !ok {
+		return nil, fmt.Errorf("Task %q not found", taskKey)


Please use TaskNotFoundError type.
See: https://github.com/mesg-foundation/core/blob/dev/service/error.go#L28-L36

NicolasMahe · 2018-08-08T05:03:34Z

service/cast.go

+	for key, value := range taskData {
+		inputType, ok := task.Inputs[key]
+		if !ok {
+			return nil, fmt.Errorf("Task %q has no input named %q", taskKey, key)


Please create a new type for this error in service/error.go

NicolasMahe · 2018-08-08T05:05:15Z

service/cast.go

+			}
+			m[key] = b
+		case "Object":
+			return nil, fmt.Errorf("Task %q - input %q - Object type is not supported", taskKey, key)


Can we really not pass any Object with the CLI? Even a JSON stringified?

At what level do you want to block it?

Someone can pass --data myObject=.... and we need to check the type and cast it.

I could be ok not to support this yet, we also have an issue to remove this and add more details on this object so it's ok if for now we don't support it.

But anyway there is no validation on the API so we could send anything so maybe we can just bypass this instead of returning an error

Ok, so I'll continue here instead of return an error

antho1404 · 2018-08-08T12:18:28Z

service/cast.go

+}
+func castObject(value string) (interface{}, error) {
+	// do not cast object type
+	return nil, nil


This is not returning any value so any service with object will fail.

We can add a basic support by at least returning the value or even better trying to parse it from string

…ues/256

antho1404

All good for me, nice job 👍

NicolasMahe

Good job 👍

antho1404 added the in progress label Aug 6, 2018

krhubert mentioned this pull request Aug 6, 2018

Add data to the service execute with no file #256

Closed

NicolasMahe reviewed Aug 6, 2018

View reviewed changes

krhubert force-pushed the issues/256 branch from 80fff56 to fc75e9b Compare August 6, 2018 17:31

NicolasMahe requested changes Aug 7, 2018

View reviewed changes

NicolasMahe removed the in progress label Aug 7, 2018

krhubert force-pushed the issues/256 branch 2 times, most recently from c752a0e to c1fdd26 Compare August 7, 2018 19:36

NicolasMahe reviewed Aug 8, 2018

View reviewed changes

krhubert force-pushed the issues/256 branch 2 times, most recently from 9ae5b4e to 01210f5 Compare August 8, 2018 09:56

NicolasMahe previously approved these changes Aug 8, 2018

View reviewed changes

krhubert added 4 commits August 8, 2018 13:32

Add Cast method to Service

b785617

Support data flag in service execute cmd

8bb78b9

Update docs and changelog

abd3d0e

Non-zero exit on error

63ab1bd

krhubert dismissed NicolasMahe’s stale review via 63ab1bd August 8, 2018 11:32

krhubert force-pushed the issues/256 branch from 01210f5 to 63ab1bd Compare August 8, 2018 11:32

antho1404 reviewed Aug 8, 2018

View reviewed changes

antho1404 and others added 3 commits August 9, 2018 00:19

Merge branch 'dev' into issues/256

e725701

Cast Object from JSON

4cc829a

Merge branch 'issues/256' of github.com:mesg-foundation/core into iss…

75d676e

…ues/256

antho1404 approved these changes Aug 8, 2018

View reviewed changes

NicolasMahe approved these changes Aug 9, 2018

View reviewed changes

NicolasMahe merged commit 2c7175d into dev Aug 9, 2018

NicolasMahe deleted the issues/256 branch August 9, 2018 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data to the service execute with no file #344

Add data to the service execute with no file #344

krhubert commented Aug 6, 2018 •

edited by NicolasMahe

Loading

NicolasMahe left a comment

NicolasMahe Aug 6, 2018

krhubert Aug 6, 2018

antho1404 commented Aug 6, 2018

krhubert commented Aug 6, 2018

antho1404 commented Aug 7, 2018

NicolasMahe left a comment •

edited

Loading

NicolasMahe Aug 7, 2018 •

edited

Loading

krhubert Aug 7, 2018

NicolasMahe Aug 7, 2018

NicolasMahe commented Aug 7, 2018

krhubert commented Aug 7, 2018 •

edited

Loading

NicolasMahe commented Aug 7, 2018 •

edited

Loading

NicolasMahe commented Aug 7, 2018 •

edited by krhubert

Loading

antho1404 commented Aug 7, 2018 •

edited by NicolasMahe

Loading

NicolasMahe Aug 8, 2018

krhubert Aug 8, 2018

NicolasMahe Aug 8, 2018

krhubert Aug 8, 2018

NicolasMahe Aug 8, 2018

krhubert Aug 8, 2018

antho1404 Aug 8, 2018

krhubert Aug 8, 2018

antho1404 Aug 8, 2018

antho1404 left a comment

NicolasMahe left a comment

Add data to the service execute with no file #344

Add data to the service execute with no file #344

Conversation

krhubert commented Aug 6, 2018 • edited by NicolasMahe Loading

NicolasMahe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antho1404 commented Aug 6, 2018

krhubert commented Aug 6, 2018

antho1404 commented Aug 7, 2018

NicolasMahe left a comment • edited Loading

Choose a reason for hiding this comment

NicolasMahe Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasMahe commented Aug 7, 2018

krhubert commented Aug 7, 2018 • edited Loading

NicolasMahe commented Aug 7, 2018 • edited Loading

NicolasMahe commented Aug 7, 2018 • edited by krhubert Loading

antho1404 commented Aug 7, 2018 • edited by NicolasMahe Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antho1404 left a comment

Choose a reason for hiding this comment

NicolasMahe left a comment

Choose a reason for hiding this comment

krhubert commented Aug 6, 2018 •

edited by NicolasMahe

Loading

NicolasMahe left a comment •

edited

Loading

NicolasMahe Aug 7, 2018 •

edited

Loading

krhubert commented Aug 7, 2018 •

edited

Loading

NicolasMahe commented Aug 7, 2018 •

edited

Loading

NicolasMahe commented Aug 7, 2018 •

edited by krhubert

Loading

antho1404 commented Aug 7, 2018 •

edited by NicolasMahe

Loading