Skip to content
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.

Sign up for GitHub

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

Jump to bottom

client,providers: add storj DCS as provider option #18

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

client,providers: add storj DCS as provider option #18

wants to merge 10 commits into from

Conversation

stefanbenten
Copy link

@stefanbenten stefanbenten commented Aug 7, 2022 •
edited
Loading

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Adding Storj Provider
  • Fixed import ordering for some touched files to make them consistent
  • Fixed some smaller items in the README.
  • Added the missing error checks in the cfggen script.

Verification

  • Compiled and ran it locally, worked fine.
  • Still working on adding the "Local Test Network" to ensure full functionality.

@brancz
Copy link
Member

brancz commented Aug 8, 2022

I have practically no experience with storj, but one of the first things it says in the docs is that it’s S3 compatible, so I wonder what a direct integration does better than using the existing S3 integration?

@stefanbenten
Copy link
Author

Hello @brancz, thank you for looking at this!

The native integration offers several advantages over the S3 compatible gateway.
One of them being performance (cuts out the proxy/gateway) and the other important one being client side encryption rather than have it be done server side.

@brancz
Copy link
Member

brancz commented Aug 18, 2022 •
edited
Loading

This generally looks good to me. I wonder though if you'd be up for maintaining this, as if there are problems I don't think any of us have enough knowledge about Storj to be helpful in a meaningful way.

Would you be up for that?

@stefanbenten
Copy link
Author

Thank you a lot @brancz !

I am more than happy to maintain the integration, let me know if there is anything further needed!

GiedriusS
GiedriusS reviewed Aug 25, 2022
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any containers that we could run to start a new storj object storage? Or is it a proprietary product? It would be ideal to have some integration tests.

providers/storj/storj.go Outdated Show resolved Hide resolved
scripts/cfggen/main.go
@@ -70,19 +72,19 @@ func main() {

errLogger := level.Error(log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)))
if _, err := app.Parse(os.Args[1:]); err != nil {
errLogger.Log("err", err)
_ = errLogger.Log("err", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to check for err here in Log() calls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mainly a thing that my environment complained about. I saw it wasnt needed after all, but thought it might be a little cleaner anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiedriusS GiedriusS GiedriusS left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stefanbenten @brancz @GiedriusS