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

Add simple object storage package for s3 and swift #89

Merged
merged 5 commits into from
Aug 16, 2019

Conversation

Soulou
Copy link
Member

@Soulou Soulou commented Jul 19, 2019

No description provided.

@Soulou Soulou requested a review from johnsudaar July 19, 2019 16:51
Copy link

@johnsudaar johnsudaar left a comment

Choose a reason for hiding this comment

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

It would be nice if the common interface and its mock would be in the package too.

storage/s3.go Outdated
s3config := s3Config(cfg)
return &S3{
cfg: cfg, s3client: s3.New(s3config), s3uploader: s3manager.NewUploader(s3config),
retryWaitDuration: time.Second, retryAttempts: 3,

Choose a reason for hiding this comment

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

Shouldn't those two fields be configurable ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually after thinking I'm wondering if it's not the package use who should be responsible of retrying a Size() command (waiting for eventual consistency right after an upload) What do you think?

Choose a reason for hiding this comment

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

mmmh if the service is eventually consistent i find it pretty nice that the package offer this option. Otherwise it should be configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm ok :-)

@Soulou
Copy link
Member Author

Soulou commented Jul 23, 2019

It would be nice if the common interface and its mock would be in the package too.

Ok will do

@Soulou
Copy link
Member Author

Soulou commented Aug 15, 2019

mock added

@Soulou Soulou requested a review from johnsudaar August 16, 2019 08:18
@johnsudaar johnsudaar merged commit 08bc672 into master Aug 16, 2019
@EtienneM EtienneM deleted the feature/storage branch June 2, 2021 07:22
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.

2 participants