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.

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

Custom S3 Endpoint #2551

Closed
wants to merge 0 commits into from
Closed

Custom S3 Endpoint #2551

wants to merge 0 commits into from

Conversation

codynhat
Copy link
Contributor

@codynhat codynhat commented Nov 11, 2022

Description

Adds a configuration option for s3-endpoint to add a custom S3 endpoint to S3StateStore using S3LevelDown.

@stbrody

How Has This Been Tested?

I have built my own Docker image (codynhat/js-ceramic:linux) and test it in a Kubernetes cluster running on Digital Ocean. It works successfully with Digital Ocean's S3 compatible "Spaces".

PR checklist

Before submitting this PR, please make sure:

  • I have tagged the relevant reviewers and interested parties
  • I have updated the READMEs of affected packages
  • I have made corresponding changes to the documentation

References:

#2495

@avious00
Copy link

@codynhat you're the man. just added spencer as a reviewer and put this in the backlog to make sure it gets reviewed & merged

@mattdavis0351
Copy link

@stbrody friendly ping since this has been a week. also finding it weird that the ci seems to be stalled when I look at this pr, seems like the dispatch event from circle may not be being reported?

@stbrody
Copy link
Contributor

stbrody commented Nov 23, 2022

Hey Cody, sorry for the delay. There is a big refactoring happening right now that affects the S3 state store. Unfortunately this PR will probably need to be reworked a bit to adapt to the new code structure. Probably should wait a week or two for the refactoring to finish, then rebase this PR and resubmit. Sorry about that, just unfortunate timing as this code had been untouched for a long time until quite recently.

CC @pawartur who is driving the refactoring of the S3 state store.

@codynhat
Copy link
Contributor Author

Sounds good. We will continue to use our fork for now and take another look once the refactor is complete.

@avious00
Copy link

@codynhat i just pinged artur, who's working on the state store refactoring, about whether we're ready to merge. thanks for continuing to commit

@codynhat
Copy link
Contributor Author

@codynhat i just pinged artur, who's working on the state store refactoring, about whether we're ready to merge. thanks for continuing to commit

Sounds good, thanks.

@pawartur pawartur self-requested a review December 13, 2022 18:56
@pawartur
Copy link
Contributor

Hey, @codynhat :). Thanks for preparing the PR. As we don't have automated tests yet for the S3 storage option, I'm gonna fetch this tomorrow and test manually.

One thing that comes to mind after reading the code is that perhaps it would be good to only pass the AWSSDK param, if the custom endpoint is defined (here), but I'll test it and leave a code suggestion, if necessary.

@pawartur pawartur removed the request for review from stbrody December 14, 2022 14:24
@pawartur
Copy link
Contributor

Ok, tested, works :). Thanks @codynhat . I'm not sure why the jobs are hanging, though. Need to check that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants