Finish implementation of Spaces API

[amoeba]

#138 added support for part of the Spaces API and we decided in #136 to start a fresh issue to help divvy up the tasks of completing the API. Below is a checklist with items transcribed from the Spaces API docs:

• Bucket Operations
  • Create a Bucket
  • List All Buckets
  • List a Bucket's Contents
  • Delete a Bucket
  • Get a Bucket's Location
  • Get a Bucket's ACL
  • Set a Bucket's ACL
  • Get a Bucket's CORS
  • Set a Bucket's CORS
  • Delete a Bucket's CORS (new)
  • Get a Bucket's Lifecycle Rules (new)
  • Configure a Bucket's Lifecycle Rules (new)
  • Delete a Bucket's Lifecycle Rules (new)
• Object Operations
  • Get an Object
  • Get Information About an Object
  • Upload an Object (PUT)
  • Copy an Object
  • Get an Object's ACLs
  • Set an Object ACLs
  • Delete an Object
  • Begin a Multi-part Upload
  • Upload a Part
  • List Parts
  • Complete a Multi-part Upload
  • Cancel a Multi-part Upload

Let me know if the organization of this list doesn't make sense or isn't useful for tackling this.

[amoeba]

@sckott I more or less copied the list from the API docs. Does having analogsea implement all of these seem appropriate?

@Thercast Did you have any preference for which methods you wanted to tackle first? I should be able to take a look at this some evening this week and actually be helpful.

[sckott]

I think it makes sense to include all of them

[amoeba]

I sat down to get a lay of this and everything looks very reasonable. I wrote a get_object function to match the Spaces API method GET ${BUCKET}.${REGION}.digitaloceanspaces.com/${OBJECT_KEY} using aws.s3::get_object and immediately noticed that aws.s3 didn't reasonably handle error cases.

See this example with an invalid bucket name, purposely run:

> rawToChar(space_get_object("welcome.html", "test-analogseas"))
[1] "<?xml version=\"1.0\" encoding=\"UTF-8\"?><Error><Code>NoSuchBucket</Code><BucketName>test-analogseas</BucketName><RequestId>tx000000000000002f5f59e-0059ffb7c0-3f970-nyc3a</RequestId><HostId>3f970-nyc3a-nyc</HostId></Error>"

It returns the raw response and throws away the underlying HTTP request's status code so there appears to be no way to know if this is the file I have stored at welcome.html or an error.

Looking at the source I can see why. Looking at the open Issues on the repo, I see that the function isn't really written to handle errors see this issue.

If it's the case that the aws.s3 package doesn't handle errors, I wonder if we (1) care and (2) would rather try to stay with using aws.s3 or use crul to get proper error handling.

Thoughts?

[rpodcast]

@amoeba thanks for your work on this! I've been AFK due to illness but I want to jump back in to help out. I'll add notes on which features I would like to tackle this week.

Regarding the error handling with aws.s3: I struggled with how to handle this in my first implementation. We have a lot of functionality already built for us within aws.s3, and having that convenience for this early implementation of spaces is hard to pass up. But showing just the raw response when the request errors out is a real eye sore compared to how the other analogsea functions elegantly handle errors via crul. From my perspective, I could see the long term path being a re-write of the key functionality using crul to be consistent with the rest of the package and having one less dependency, but I will defer to @sckott for his take.

[sckott]

Thanks for raising the issue with erroring. I definitely value failing well, so I'd strongly prefer to make sure users get useful errors - and DigitalOcean, at least with the Droplet API (and I assume with the spaces API) fails well and correctly making it easy to give users appropriate errors

We currently still use httr in this package. Main thing keeping httr around is that it can do oauth whereas crul cannot, but i think an argument can be made to get rid of oauth if we think most people don't use it but instead use PATs

For now I think it's okay to stick with aws.s3, but eventually i think we should rework to make sure spaces fxns error well

[amoeba]

Sounds good, thanks for weighing in @sckott. If/when the aws.s3 package changes how they handle error cases, this package benefits automatically, so I'm down with filling in this portion of the package with aws.s3 as much as possible.

@Thercast Sorry to hear you were out! Looking forward to your notes.

[sckott]

any thoughts on this? anything I can help with?

[amoeba]

I'm still down to help PR this addition but wanted to let @Thercast take lead since it was his idea. Hopefully he'll pop in here and we can get this done soon?

[sckott]

@Thercast any thoughts? if you're too busy, anything @amoeba or I can move forward on?

[rpodcast]

Hi @sckott and @amoeba, sorry for the delay (an urgent shiny app came up at work and then the kids got sick) but I would like to tackle the listing of bucket contents and deletion of a bucket (these tasks should help shake off the cobwebs of being away for a bit). @amoeba I'd be glad if you'd like to start on the fundamental operations for objects in a space (i.e. downloading, uploading, deleting). Does that sound like a good plan?

[amoeba]

Sounds good to me!

[amoeba]

First half of the Object calls implemented in #146. Went pretty well. Regarding the calls I didn't implement,

• Delete an Object

  I don't know why my code isn't working (yet). It looks like I'm seeing an issue with passing of the base_url argument which is only affecting this function. It looks like it should work just fine.

• Multi-part operations

  aws.s3 implements what appears to be the same multi-part uploading API that Spaces does but they do it as an argument multipart on put_object which handles the Begin/Upload/Complete calls. Should each of those Spaces API calls to have their own functions or should we just calls this done?

General questions:

• Function names: What do we think?
• Testing: Do we want mocks here so we have at least some basic regression tests in place? I didn't se mocks in other places so skipped this for now
• Docs: I tried to make them fit stylistically. Thoughts?
• Should some of these calls get split into one or more files?

I'll look into the delete operation issue above later this week.

[sckott]

Regarding the PR, I assume you're adding more to it? Or do you want it to be handled as is?

• do you have thoughts on function names? I think it would be better to change the structure to e.g. spaces_thing_verb, so for example change spaces_get_object to spaces_object_get so that when users are searching through function names (e.g., tab completion to find functions) you get all the functions for spaces_object*, or for spaces_acl*
• tests: yeah, tests are sorely lacking here. been meaning to 1) add a few live tests of droplet creation/etc., but 2) mostly test against mocked/cached data. I'll open a new issue for this to discuss it there
• docs:
  • the repeated parameters in each spaces fxn could go into man-roxygen/ as a template file to save lines
  • wrap booleans in \code{}
  • maybe for examples, e.g., for each man file, include entire workflow from creating a space to doing whatever the task is (e.g. spaces_get_object)
• Yes, I think breaking up spaces fxns into many files is good