Skip to content
This repository was archived by the owner on Oct 30, 2018. It is now read-only.

Add support for running scripts (ScriptEndpoint) via CLI.#28

Merged
opalczynski merged 9 commits intoSyncano:developfrom
coding-buzz:develop
Jun 20, 2016
Merged

Add support for running scripts (ScriptEndpoint) via CLI.#28
opalczynski merged 9 commits intoSyncano:developfrom
coding-buzz:develop

Conversation

@ilu2112
Copy link
Copy Markdown
Contributor

@ilu2112 ilu2112 commented Jun 6, 2016

Closes issue #27. To test it manually simply type in syncano execute --help and follow the syntax.

@opalczynski
Copy link
Copy Markdown
Contributor

@ilu2112 please correct isort issues:

ERROR: /home/ubuntu/syncano-cli/syncano_cli/main.py Imports are incorrectly sorted.
ERROR: /home/ubuntu/syncano-cli/syncano_cli/execute/commands.py Imports are incorrectly sorted.
ERROR: /home/ubuntu/syncano-cli/build/lib/syncano_cli/main.py Imports are incorrectly sorted.
ERROR: /home/ubuntu/syncano-cli/build/lib/syncano_cli/execute/commands.py Imports are incorrectly sorted.

Comment thread syncano_cli/execute/commands.py Outdated
except NoOptionError:
LOG.error('Do a login first: syncano login.')
sys.exit(1)
api_path = ScriptEndpoint._meta.endpoints['run']['path'].format(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would be nicer:

se = ScriptEndpoint.please.get('instance-name', 'script-name')
se.run()
response = se.run(**payload)
# process (if needed) and print response here

@ilu2112
Copy link
Copy Markdown
Contributor Author

ilu2112 commented Jun 7, 2016

@opalczynski It should be fine now, I have carried all Your suggestions for changes.

@opalczynski
Copy link
Copy Markdown
Contributor

opalczynski commented Jun 8, 2016

@ilu2112 Sorry that writing so lately - but yesterday was a busy day. It's almost good :) I've checked it and have some proposals:

A. Sometimes the --payload argument is not required at all, so you need to check if it was provided.

syncano_cli$ python main.py execute hidden-lake-9362 test123
Traceback (most recent call last):
  File "main.py", line 36, in <module>
    main()
  File "main.py", line 29, in main
    cli(obj={})
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/sopalczy/projects/syncano/cli_prs/syncano-cli/syncano_cli/execute/commands.py", line 35, in execute
    data = json.loads(payload.strip() or '{}')
AttributeError: 'NoneType' object has no attribute 'strip'

B. You should definitely wrap json.loads() in try catch block and log nice error.

syncano_cli$ python main.py execute hidden-lake-9362 test123 --payload "{'one': 1}"
Traceback (most recent call last):
  File "main.py", line 36, in <module>
    main()
  File "main.py", line 29, in main
    cli(obj={})
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python2.7/dist-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/sopalczy/projects/syncano/cli_prs/syncano-cli/syncano_cli/execute/commands.py", line 35, in execute
    data = json.loads(payload.strip() or '{}')
  File "/usr/lib/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python2.7/json/decoder.py", line 380, in raw_decode
    obj, end = self.scan_once(s, idx)
ValueError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

C. I think that it would be nice to format the output different.

syncano_cli$ python main.py execute hidden-lake-9362 test123 --payload {}
{
    "stderr": "", 
    "stdout": "1212"
}


syncano_cli$ python main.py execute hidden-lake-9362 test123 --payload {}
{
    "stderr": "Traceback (most recent call last):\n  File \"/app/source/main.py\", line 24, in <module>\n    2/0\nZeroDivisionError: integer division or modulo by zero", 
    "stdout": ""
}

First example above is when everything is good with script - then simply log response.result['stdout']

Second - is when some error occured: then we should output something like this:

ERROR: "{error}".format(error=response.result['stderr'])

Comment thread syncano_cli/execute/commands.py Outdated
sys.exit(1)
instance = connection.Instance.please.get(instance_name)
se = instance.script_endpoints.get(instance_name, script_endpoint_name)
data = json.loads(payload.strip() or '{}')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wrap this in try catch and log error if occurs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

additionally: payload can be None here - so check it first;

@opalczynski
Copy link
Copy Markdown
Contributor

This is really good! But can you update the README file with this new command?

@ilu2112
Copy link
Copy Markdown
Contributor Author

ilu2112 commented Jun 9, 2016

Yes, I will. I will also adapt logging to changes from pull request #32.

Comment thread syncano_cli/execute/commands.py Outdated
data = json.loads((payload or '').strip() or '{}')
response = se.run(**data)
if response.status == 'success':
print(json.dumps(response.result['stdout'], indent=4, sort_keys=True))
Copy link
Copy Markdown
Contributor

@opalczynski opalczynski Jun 13, 2016

Choose a reason for hiding this comment

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

I think You're almost there! :)

In Syncano script user can define their own custom response - then the field stdout is ignored and result is palces in response key, take a look at examples:

{u'response': {u'status': 200, u'content': u'{"one": 1}', u'content_type': u'application/json'}, 
u'stderr': u'', u'stdout': u''}
# output: "" (the print(json.dumps(....)))

{u'stderr': u'', u'stdout': u'Some string'}
# output: "Some string"

{u'stderr': u'', u'stdout': u'16'}
# output: "16"

{u'stderr': u'Traceback (most recent call last):\n  File "/app/source/main.py", line 24, in <module>\n    
2/0\nZeroDivisionError: integer division or modulo by zero', u'stdout': u''}
# output: ""

{u'response': {u'status': 200, u'content': u"{'one': 1}", u'content_type': u'applicattion/json'}, 
u'stderr': u'', u'stdout': u''}
# output: ""

Also the json.dumps for simple data types will wrap it with "": 12 -> "12" I think this is not needed here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more thing: the custom response can be returned with standard response:

{u'response': {u'status': 200, u'content': u'{"one": 1}', u'content_type': u'application/json'}, 
u'stderr': u'', u'stdout': u'12'}

LOG = get_logger('syncano-execute')


def print_response(response):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about response.result['response'] ?

Copy link
Copy Markdown
Contributor Author

@ilu2112 ilu2112 Jun 13, 2016

Choose a reason for hiding this comment

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

@opalczynski I don't understand You, what do You mean? I see no response.result['response'] field in response.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider this - this is a script code:

import json

set_response(HttpResponse(status_code=200, content=json.dumps({'one': 1}), content_type='application/json'))

print(12)

This:

s = Script.please.get(id=1)

t = s.run()

t.reload()
print(t._raw_data)

Will output:

{
    'status': u'success', 
    'links': <syncano.models.fields.LinksWrapper object at 0x7fd2dd91d690>, 
    'executed_at': datetime.datetime(2016, 6, 13, 12, 50, 23, 99456), 
    'instance_name': 'hidden-lake-9362', 
    'result': {
        u'response': {
            u'status': 200, 
            u'content': u'{"one": 1}', 
            u'content_type': u'application/json'}, 
        u'stderr': u'', 
        u'stdout': u'12'}, 
    'duration': 38, 
    'script_id': 1, 
    'id': 93
}

I can't see in your solution handling of the custom response - maybe I've missed something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're talking about ScriptEndpoint, right? Your script's returns

{
    "one": 1
}

if executed from syncano-cli. Please check it Yourself.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To clarify. ScriptEndpoint will return the actual custom response as @ilu2112 mentioned. Trace for it will include the structure @opalczynski mentioned. But here I believe we are only dealing with script endpoints so it seems ok: either structure has a result or should be returned as-is.

Copy link
Copy Markdown
Contributor

@opalczynski opalczynski Jun 13, 2016

Choose a reason for hiding this comment

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

Yep, my bad :) sry about this. It looks good now :)

@opalczynski
Copy link
Copy Markdown
Contributor

👍

@opalczynski
Copy link
Copy Markdown
Contributor

@ilu2112 Is this ready? :) If so - please resolve merge conflicts and merge ;)

@opalczynski opalczynski merged commit ebe443f into Syncano:develop Jun 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants