feat: add get_log(job)

[toph-allen]

Intent

The initial intent of this PR was to move get_job() to use the v1 API.

However, that endpoint returns the exact same data as get_jobs(), so it seems kinda redundant. Is there any point at which you'll have a job key where you haven't called get_jobs()? Not right now, I don't think.

The old unversioned get_job() function also returned the log for the job, and this seems to be the main use case for the singular function, so I instead added a new function, get_log(), which gets the job log.

Fixes #341

Approach

The new function is get_log(content_item, key). It's a little clunky to use, because the API requires the content GUID as well as the job key.

An alternate approach would be to add a Job R6 class. This would also probably also require a different method / signature for things like terminate_jobs(), because you'd wanna keep the existing implementation but also allow you to call terminate_jobs(job_class) (although that plural function name suggests that it takes a list of jobs, so maybe it would just call for a terminate_job(job_class_object) function.

• Adds a get_job_list(content) function which returns a list of jobs for a content item. Each job is just a list containing the API output, augmented with the app_guid (which is required for subsequent job requests but not contained in the data returned) and the Connect client.
• Adds a get_log(job) function which returns the job log for the job object as represented by that list item.

client <- connect()
content <- content_item(CONTENT_GUID)
jobs <- get_job_list(content)
log <- get_log(jobs[[1]])

Checklist

• Does this change update NEWS.md (referencing the connected issue if necessary)?
• Does this change need documentation? Have you run devtools::document()?

[jonkeane]

Thanks, a few questions inline.

An alternate approach would be to add a Job R6 class. This would also probably also require a different method / signature for things like terminate_jobs(), because you'd wanna keep the existing implementation but also allow you to call terminate_jobs(job_class) (although that plural function name suggests that it takes a list of jobs, so maybe it would just call for a terminate_job(job_class_object) function.

Could you say more about this? What would the benefits of this approach be? What about the downsides? It might also be helpful here to write the code that someone using this would write if this were the case and compare it to the code that one would right with this PR as it stands.

[toph-allen]

@jonkeane Can I get a re-review when you have a sec?

[jonkeane]

Thanks for this. The list-based approach looks interesting and a promising direction to try out.

One question I have is how much should we flag this tension / split to folks, especially (also commented below in the right place in the code):

Is one able to use get_job_log() with the output of get_jobs()? I think the answer is no — that's ok, but we should probably flag that.

Having two separate paths a list-based one and a data.frame-based one is totally fine, but if we do that we should embrace it + document it.

[jonkeane]

If we include this, we should do it elsewhere, but should we talk a little bit about this difference in the documentation? Where to use one or the other?

[toph-allen]

It would be reasonable to talk about this elsewhere. This might be a good place to talk about it, or it might make sense in the "details" section of this documentation.

I think we also might want to add a terminate_job() function that takes a single job object, analogous to get_job_log(). The current terminate_jobs() function takes a content item and an optional job key.

---

Aside — the reason I haven't used get_log() for this function is because Connect has multiple types of logs (e.g. audit, server) and

[toph-allen]

I wrote a paragraph in the Details section of this functions documentation to get at the distinction. When you say "we should do it elsewhere" did you mean "outside of the function documentation" or "outside of the @return section"?

I want to add a terminate_job() function in a small follow-on PR that also takes job objects. That's another example to add to this documentation that helps to get at the distinction. The current terminate_jobs() function is built more for the data frame paradigm.

[jonkeane]

Is one able to use get_job_log() with the output of get_jobs()? I think the answer is no — that's ok, but we should probably flag that.

[toph-allen]

The docs already already mention this, in the text for the job param to get_job_log(). You commented here — does that indicate you think there should be, like, a comment in the code block here?

[jonkeane]

Ah no, I see it now. I missed it on first read. Maybe that means it should be highlighted more (especially given the intentional split we are making here), or maybe it means I need better glasses. I'll leave it up to you if you want to call that out more highly (like in the body text of help).

FWIW, I commented here because it's where I thought about it looking at the examples and missed it in the argument which would have been a better place to comment it on. I might be reading slightly too much into word choice, but I agree with the (subtle) skepticism I'm reading about it being in a comment in the example as a way to highlight that.

[toph-allen]

I made a minor change to the documentation to mention this in more places and make it slightly more prominent. I believe these were the only issues you had in your review — as such I requested re-review.

But I think this gets at the major sticking point for me with the "it's just a list" approach — which is that there is no good way to go from the data frame returned by get_jobs() to the object needed by get_job_log(). Without this, I think it's likely that we haven't fully thought through our users' workflows, and are building something that looks good in minimal examples but doesn't provide good ergonomics beyond that.

I'm fine merging this PR as is — documenting that limitation — I think it's best to move forward here. But this is something that I've been thinking about e.g. in #305.

---

One thought I had as a stepping stone in that direction, which almost works with what connectapi currently has, is a job() function that takes a job key and returns the server data (the stand-in for a "job" class).

The workflow I have in mind is:

client <- connect()

client |>
  content_item(GUID) |>
  get_jobs() |> # data frame
  slice_max(end_date) |> # get the most recent
  pull(key) |>
  job() |> # construct a job object — in this case just a list
  get_job_log()

But job() would need the client, and that's so far always the first argument, so this workflow would need to break that paradigm. This is why I've been thinking about #359 (default client as a trailing arg). This would require a surplus HTTP request but that seems fine.

Another option that might actually work is just piping a single-row data frame to as.list() — except that the data frame lacks a list-column of client objects, i.e. it's not an exact match for the data from get_job_list().

client <- connect()

client |>
  content_item(GUID) |>
  get_jobs() |> # data frame
  slice_max(end_date) |> # get the most recent
  as.list() |> # wouldn't actually work right now
  get_job_log()

[toph-allen]

@jonkeane Thinking it would be better to just call this jobs(), which would align it with #305.

In other words — if a function returning a list-like collection of a Connect server objects is an evolutionary transition fossil on the way to an API like that described in #305, we should name it like that.

[jonkeane]

That sounds like a great idea

[toph-allen]

I actually misremembered the proposal when I was writing that comment on Sunday. The actual resource class is not the thing which could be accessed in list-like ways. That proposal uses a function get_all() (called on the resource class) to get a list of objects.

I think I'd actually want to avoid using a jobs() function name to leave it unoccupied for a resource class, should one be implemented. I was trying to think about whether it's reasonable to have a resource class that could be addressed like a list, but that's out of scope.

[toph-allen]

In practice, having a function called jobs() that returns a list and a function called get_jobs() that returns a database is less intuitive than frustratingly wordy function names.

[toph-allen]

@jonkeane I made some minor documentation changes responding to your review, but git pull/push operations are not working (on any repo) and I'm going down a rabbit hole trying to fix. :|

[edit] https://www.githubstatus.com/incidents/qd96yfgvmcf9

[toph-allen]

Merging without approval after checking in with @jonkeane.