Addition of Amazon ECS job, copied code from luigi.contrib.ecs.py and… #13
Conversation
… modified for ndscheduler JobBase.
… modified for ndscheduler JobBase.
… modified for ndscheduler JobBase. Fixed the 100 character limit
… modified for ndscheduler JobBase. Fixed the 100 character limit blank whitespace
| @@ -0,0 +1,122 @@ | |||
| """A sample job that prints string.""" | |||
There was a problem hiding this comment.
How do people pass AWS credential? Better add some operational guidelines here, e.g., create dot file on worker nodes for aws credential .
simple_scheduler/jobs/ecs_job.py
Outdated
| import boto3 | ||
|
|
||
| client = boto3.client('ecs') | ||
| except ImportError: |
There was a problem hiding this comment.
IMHO, don't catch error here. Let the error bubble up. If boto is not imported, let it fail hard.
simple_scheduler/jobs/ecs_job.py
Outdated
|
|
||
| # Error checking | ||
| if response['failures']: | ||
| raise Exception('There were some failures:\n{0}'.format( |
There was a problem hiding this comment.
Don't raise a generic exception. Make a custom exception that subclasses Exception, e.g., ECSJobException.
|
Thanks, I'll update. I agree about the Exceptions and boto3 try/catch. Like I said before in the original comment this was copied from luigi.contrib.ecs, so there is definitely some mods that are needed. |
|
Yeah I'm using PyCharm so some of that is built-in, it's length limit is set to 160 characters. It looks like there are some other issues with 2 other files that I didn't edit. |
There was a problem hiding this comment.
Looks good to me.
Change the file comment for simple_scheduler/jobs/ecs_job.py and fix the "make flake8" error, then I'll approve it :)
No sure why there are "make flake8" errors from other files you didn't touch. But they are pretty easy to fix (e.g., adding one new blank line).
|
@rhaarm could you pull and merge from latest master? That should fix all the flake8 error & fixed unit tests for python 3.5. Then this pull request should be good to go |
… modified for ndscheduler JobBase.