search

pachadotdev / analogsea

Digital Ocean R client
https://pacha.dev/analogsea/
Apache License 2.0
154 stars 24 forks source link

Thoughts on spaces API integration #136

Closed rpodcast closed 6 years ago

rpodcast commented 7 years ago

I'm a big fan of analogsea as digital ocean is a huge part of my infrastructure, so thank you so much for creating this package! Recently digital ocean has begun rolling out early access to their new object storage capability (also known as spaces) and I'm fortunate enough to be part of the early beta for access. It operates in a similar way to amazon S3; in fact they tried to make their spaces API quite compatible with the amazon S3 api. I see great potential for this feature and would be glad to work with you on implementing this in analogsea if you are interested.

sckott commented 7 years ago

@thercast Glad you like the package.

This sounds good to me! I just applied for it as well so that I can contribute to development

How do you want to proceed? Do you have time to work on the code for this, or do you want me to start on it? If you start on it, do follow general style we've used in this package

rpodcast commented 7 years ago

Thanks @sckott! I am interested in providing development help (I've been looking to contribute to R packages that wrap APIs) but wanted to get your opinion on whether we should try to leverage some of the functions from the aws.s3 package, given the compatability of the API? I will investigate aws.s3 in more detail to get a better idea of the functionality it offers.

sckott commented 7 years ago

okay

sckott commented 6 years ago

any thoughts on this?

amoeba commented 6 years ago

[Commenting because I had been lurking on this thread for a few weeks] I'd be down with helping implement this if the help was needed.

rpodcast commented 6 years ago

AFK but I like your idea of passing the 404 in a friendly way back to the user. In that way we don't need to keep track of which functions need auth versus not. I'm still learning the ropes on best practices, but it seems with httr we should be able to get the status of the request back and configure a stop with a simple message based on it

On Sep 26, 2017 6:00 PM, "Scott Chamberlain" notifications@github.com wrote:

any thoughts on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sckott/analogsea/issues/136#issuecomment-332348722, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_qp6pqDvjH6320aIwyOQ-mPQmzQY6uks5smXPigaJpZM4PKblJ .

sckott commented 6 years ago

@thercast i think you meant to put that in discgolf?

rpodcast commented 6 years ago

Oh geez yes you are right! Regarding this I looked more closely at the aws.s3 package and while it would be a bit much to depend on it directly (since all their functions require aws credentials) I think we can adapt most of the key functions to list, add, delete objects from spaces. I'll try implementing a method to list all spaces for a user following your structure for the package

On Sep 26, 2017 6:08 PM, "Scott Chamberlain" notifications@github.com wrote:

@thercast https://github.com/thercast i think you meant to put that in discgolf?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sckott/analogsea/issues/136#issuecomment-332350588, or mute the thread https://github.com/notifications/unsubscribe-auth/AA_qpxLEd53lg_fsSrAf7NF_Wzcd_aQTks5smXXPgaJpZM4PKblJ .

sckott commented 6 years ago

@amoeba i think so, maybe wait to see first PR from @thercast then divy up work

PR's sounds good

amoeba commented 6 years ago

Sounds good to me!

rpodcast commented 6 years ago

@sckott I have preliminary support for a few of the key operations for spaces (listing spaces, listing objects within a space, and creating a new space) ready for your review at this custom branch. I have more polishing to do but didn't want to go too far in case you have different ideas.

It turns out that the aws.s3 package functions work nicely with digital ocean spaces once you set a few of the parameter values differently (such as base url and the access/secret keys). It does bring a few additional package imports (base64enc, digest, aws.signature) so that is something to consider. Also I acknowledge that these new functions stick out in terms of their overall methodology (i.e. no S3 methods/classes). If you like the general direction I'll be happy to add some additional operations (such as deleting spaces and adding/deleting objects within spaces).

sckott commented 6 years ago

@thercast

rpodcast commented 6 years ago

@sckott here are some notes:

are the key and secret for spaces different than for droplets? looks like it, just making sure

Yes that's correct. Digital Ocean allows you to create multiple spaces keys, so you can specify the key associated with a bucket during creation.

i'd prefer not to have users pass in access key and secret but to instead have them set env vars

I agree. One thing I forgot to mention is that the functions will look for environment variables DO_SPACES_ACCESS_KEY and DO_SPACES_SECRET_KEY in the session if they are not passed explicitly in the function calls. If they are not defined, then an error message appears. I defined those in ~/.Renviron in my testing.

i do want to change returned objects from fxns to be similar to what this pkg does already, e.g., the returned objects here https://github.com/sckott/analogsea#get-droplets - i think it's important to be consistent within this pkg

Makes perfect sense. With a new space object type, we can have similar methods for summary and print like the droplet object/methods. Some useful stats (like total space size) aren't exposed directly in the API, hence many s3 clients need to grab the info for all objects and then sum their file size. For small spaces that will still be manageable, but I tested with one of my spaces containing 17k objects and space('name_of_space') took approx 15 seconds.

go ahead and send a PR whenever, can always add to it/chagne it

I'll do a bit more cleanup (documentation and adding tests) and put together basic print and summary methods for spaces much like your existing methods for droplets before I send a PR.

sckott commented 6 years ago

All sounds good

sckott commented 6 years ago

Now that #138 is merged, can you two divy up tasks ti fill out remaining methods for spaces API?

amoeba commented 6 years ago

Glad to hear it. Nice work @thercast! @thercast do you want to make a new Issue with a checklist of remaining tasks and I can put my name on a few if you want help?

rpodcast commented 6 years ago

@amoeba that sounds like a great idea. Please go ahead and create a new issue and we can discuss how to divy up the remaining pieces.

sckott commented 6 years ago

closing in favor of continuing discussion in #140