-
Notifications
You must be signed in to change notification settings - Fork 505
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
pkg/storage: add External implementation #1587
Conversation
@marwan-at-work this PR will close these issues in addition to #1110 :
Can you put those 3 in the OP? Edit: it can also close #1551 |
ceed6bc
to
41f5890
Compare
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.
@marwan-at-work this looks really good!! 🚀
I think this is good as-is, and my comments could be be for follow-ups if you like. One other thing that we can also leave for a follow-up would be writing down the external storage API in docs, in case someone else wants to roll their own server
srv := httptest.NewServer(handler) | ||
defer srv.Close() | ||
externalStrg := NewClient(srv.URL, nil) | ||
clear := strg.(interface{ Clear() error }).Clear |
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.
worth having an explicit named Clearer
(or something) interface?
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.
@arschles this method is only defined/used inside _test.go
files and you cannot import it if you define it in a _test.go
file anyway. So I'd keep it this way, but if we find ourselves doing this casting in other places, then it would def be worth it to define it in a none-test file and use it from within tests similar to our compliance package.
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.
What about here?
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.
@arschles that is where the method is defined, sorry i meant the interface-casting is only defined/used in _test.go
The reason that method is defined (and got moved from _test.go) to non-test Go file, is exactly because _test.go files in other packages don't get imported even during tests.
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 got ya, this is good for me then 👍
@arschles addressed all the comments, let me know if it looks good. thanks! |
I'm looking forward to this going in! I'll start writing some storage servers after it does 😄 Let's plan to do some docs after this is merged, and before it's released 🛩 🚀 |
This PR adds an http external storage implementation. It re-uses a lot of what we have already and optimizes "save" operations by uploading all 3 files (info/mod/zip) in one endpoint as a piped multi-writer.
Basic documentation and example is included, but I'll add more docs in follow ups.
Fixes #1110
Fixes #1353
Fixes #1130
Fixes #1131
Closes #1551