-
Notifications
You must be signed in to change notification settings - Fork 115
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
Adds basic AWS S3 client #1019
Adds basic AWS S3 client #1019
Conversation
✅ Deploy Preview for elastic-ritchie-8f47f9 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This is definitely incomplete. I have not even given it a once over myself, so expect there to be silly mistakes 🙂 |
Looks generally good. I'll let vyzo decide whether we want that in gerbil itself, or in a separate pkg gerbil-aws or such. Since it's a proprietary service, I'd lean towards making it a separate package, myself. |
If not in stdlib, this desrves to be a mighty gerbils official package. Let's get it reviewed here and we decide. |
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.
looks pretty good, left some comments.
In general, if you have a contract check at the interface level (you do), you can elide it in the method implementation.
This is already in a separate repo under chifnoah/gerbil-s3, I'd be happy to clean that up and transfer ownership to the might-gerbils organization. That being said, while S3 is a proprietary service, the S3 api is definitely a de-facto standard by this point with several open source and 3rd party proprietary implementations. I think I still lean towards having it be in a separate package though. |
Don't worry about the gambit master CI failure, there has been a change in the locat constructor, I'll fix that. |
60e1c5c
to
b282178
Compare
Alright, I think it's good to go now 🙂 |
Includes support for: - Bucket get/set/list/delete/exists? - Server-side object copying - Object get/set/list/delete - Bucket exists
* Raise exceptions instead of return #f * Don't export structs, create a constructor that wraps the return value in the interface * Raise error when credentials are not supplied * memv -> memq for speed * more appropriate use of (using ...) * Renames interfaces to be more clear
* Renames s3-client constructor from make-s3-client to S3Client to not conflict with the one generated by `defstruct` * Calculate the empty-body sha256 hash on module import time
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.
LGTM, can we add some docs?
Yup! Will work on that |
src/std/net/s3/interface.ss
Outdated
(get (name :~ string?)) | ||
(put! (name :~ string?) | ||
(data :~ u8vector?) | ||
content-type: (content-type := "octet-stream" :~ string?)) |
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.
do we care about this?
Note that dynamic dispatch with kw lambdas is expensive.
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.
Do we pay that cost no matter what or only when we specify content-type:
?
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.
I guess it's not really necessary if we're enforcing (data :~ u8vector?)
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.
Yes, it is a dynamic call through the interface, so the compiler cannot optimize dispatch.
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.
I would say it is ready to merge, one minor thing -- see comment.
Includes support for: - Bucket get/set/list/delete/exists? - Server-side object copying - Object get/set/list/delete - Bucket exists Partially resolves mighty-gerbils#840 --------- Co-authored-by: vyzo <[email protected]>
Includes support for:
Partially resolves #840