-
Notifications
You must be signed in to change notification settings - Fork 9
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
mrc-3167: Add function to cancel reports running on remote #314
Conversation
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 41 41
Lines 4502 4508 +6
=========================================
+ Hits 4502 4508 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of small comments
@@ -9,6 +9,7 @@ export(orderly_bundle_list) | |||
export(orderly_bundle_pack) | |||
export(orderly_bundle_pack_remote) | |||
export(orderly_bundle_run) | |||
export(orderly_cancel_remote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be orderly_remote_cancel
to mirror orderly_remote_status
or stay orderly_cancel_remote
to mirror orderly_run_remote
? Naming things is hard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have more functions which are orderly_x_remote
, though they do tend to be when there is a local equivalent - which there isn't here. I think orderly_cancel_remote
feels more natural wording to me so I am happy to stick with that, I don't mind switching it over if you prefer though
orderly_cancel_remote <- function(keys, root = NULL, locate = TRUE, | ||
remote = NULL) { | ||
remote <- get_remote(remote, orderly_config(root, locate)) | ||
out <- lapply(keys, remote$kill) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to make this part of the "implements remote" thing, fall back here:
if (is.null(remote$kill)) {
stop(sprintf("your remote (there's a way of getting the type I believe) does not implement kill")
}
that's probably the tidy thing to do as we don't want to implement this for the path remote
orderly_cancel_remote <- function(keys, root = NULL, locate = TRUE, | ||
remote = NULL) { | ||
remote <- get_remote(remote, orderly_config(root, locate)) | ||
out <- lapply(keys, remote$kill) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to make this part of the "implements remote" thing, fall back here:
if (is.null(remote$kill)) {
stop(sprintf("your remote (there's a way of getting the type I believe) does not implement kill")
}
that's probably the tidy thing to do as we don't want to implement this for the path remote
@@ -9,6 +9,7 @@ export(orderly_bundle_list) | |||
export(orderly_bundle_pack) | |||
export(orderly_bundle_pack_remote) | |||
export(orderly_bundle_run) | |||
export(orderly_cancel_remote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be orderly_remote_cancel
to mirror orderly_remote_status
or stay orderly_cancel_remote
to mirror orderly_run_remote
? Naming things is hard
This PR will
orderly_cancel_remote
which will callkill
from orderly.server to cancel/kill a report on the remote with its key.Couple of questions
kill
function become a check inimplements_remote
? I need to add to sharepoint remote still to explicitly throw an error if trying to callkill
thereorderly_cancel_remote
as close toorderly_run_remote
vimc/orderlyweb#18 needs to be merged before this will work