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 delete CronJob support #6

Merged
merged 3 commits into from Jan 7, 2020
Merged

Add delete CronJob support #6

merged 3 commits into from Jan 7, 2020

Conversation

@tmad
Copy link
Contributor

tmad commented Jan 3, 2020

No description provided.

Copy link
Contributor

agaffney left a comment

This looks good in general, but I left a few comments

JsonError(c, fmt.Sprintf("failed to delete job: %s", err))
return
}
c.String(200, cronJobDeleted)

This comment has been minimized.

Copy link
@agaffney

agaffney Jan 3, 2020

Contributor

Does this match the semantics of this API endpoint in Metronome? The docs only show what the negative responses should look like, but they are all JSON dicts.

This comment has been minimized.

Copy link
@tmad

tmad Jan 7, 2020

Author Contributor

It was not documented - turns out it returns job details in case of DELETE. I've updated it.

- /bin/sh
- -c
- date; echo Hello from the Kubernetes cluster hello5
restartPolicy: OnFailure

This comment has been minimized.

Copy link
@agaffney

agaffney Jan 3, 2020

Contributor

I created 4 initial jobs to give us something to delete when we got to it, but it doesn't hurt to add more.

test/run-test.py Outdated Show resolved Hide resolved
if not compare_structures(r.json(), response_data):
print('Got response JSON:\n\n%s\n\nexpected:\n\n%s' % (r.text, json.dumps(response_data)))
sys.exit(1)
for i in range(retries):

This comment has been minimized.

Copy link
@agaffney

agaffney Jan 3, 2020

Contributor

I would default retries to 0 above and make this range(retries + 1) so that the number of retries refers to extra tries, rather than the overall number of retries, to keep it logically consistent.

This comment has been minimized.

Copy link
@tmad

tmad Jan 7, 2020

Author Contributor

makes sense, fixed

@tmad tmad force-pushed the delete-cronjob branch from cb4ec06 to 5c21265 Jan 7, 2020
@tmad tmad force-pushed the delete-cronjob branch from 5c21265 to 5e66405 Jan 7, 2020
@tmad tmad merged commit da2185b into master Jan 7, 2020
2 checks passed
2 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
@tmad tmad deleted the delete-cronjob branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.