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

add jobs support #95

Closed
wants to merge 6 commits into from
Closed

add jobs support #95

wants to merge 6 commits into from

Conversation

mgummelt
Copy link
Contributor

@mgummelt mgummelt commented Dec 2, 2016

No description provided.

import time
import urllib
from dcos import config, errors, http

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know Python's style rules. Should there be another newline here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

break
else:
raise ex
time.sleep(5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe want to make this a defaulted parameter to the method.

try:
response = get_run(job_id, run_id)
except errors.DCOSHTTPException as ex:
if ex.response.status_code == 404:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there somewhere that has a list of these constants? Also, I've seen a bunch of tests fail flakily due to occasional 404s returned lately. Are you sure you want to fail on 404? What if something like 502 is returned forever, does this just run forever?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I believe there's already a generic spin implementation somewhere in shakedown.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generic spinner: spinner.wait_for

Copy link
Contributor

@gabrielhartmann gabrielhartmann left a comment

Choose a reason for hiding this comment

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

Made a few comments. Address as seems appropriate to you.

return job_id


def wait_for_run(job_id, run_id, duration=5):
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like duration is used anywhere.

@mgummelt
Copy link
Contributor Author

mgummelt commented Dec 2, 2016

retest this please

2 similar comments
@mgummelt
Copy link
Contributor Author

mgummelt commented Dec 2, 2016

retest this please

@mgummelt
Copy link
Contributor Author

mgummelt commented Dec 2, 2016

retest this please

@kensipe
Copy link
Contributor

kensipe commented Dec 5, 2016

shakedown has a dependency on dcos-cli which provides access for functions and features like this. https://github.com/dcos/dcos-cli/blob/master/dcos/metronome.py is the way to enable this behavior.

@kensipe
Copy link
Contributor

kensipe commented Dec 5, 2016

the dcos-cli will be released this week and scott will provide an update in shakedown when the release happens. That makes this PR redundant.

@kensipe
Copy link
Contributor

kensipe commented Dec 5, 2016

examples of how to use are here: https://github.com/dcos/metronome/blob/master/tests/system/test_root_metronome.py

it currently uses a local copy of metronome prior to the PR being merged into dcos-cli.

Copy link
Contributor

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

This is redundant and isn't the recommended approach.

@mgummelt
Copy link
Contributor Author

mgummelt commented Dec 5, 2016 via email

@mgummelt mgummelt closed this Dec 6, 2016
@mgummelt mgummelt deleted the jobs branch December 6, 2016 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants