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

🚀 Add S3 Implementation #227

Merged
merged 18 commits into from
Feb 4, 2022
Merged

🚀 Add S3 Implementation #227

merged 18 commits into from
Feb 4, 2022

Conversation

pcen
Copy link
Contributor

@pcen pcen commented Oct 1, 2021

Adds S3 implementation of the Storage interface

@pcen pcen mentioned this pull request Oct 1, 2021
Closed
@Aliqyan Aliqyan mentioned this pull request Oct 1, 2021
Merged
ReneWerner87
ReneWerner87 reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

please add the new storage in the workflow files in the root of the repository under .github

@pcen pcen closed this Oct 5, 2021
@pcen pcen reopened this Oct 5, 2021
ReneWerner87
ReneWerner87 approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

please add the new storage here https://github.com/gofiber/storage/blob/main/README.md

ReneWerner87
ReneWerner87 requested changes Oct 11, 2021
View reviewed changes
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

please add some tests in the s3_test.go file

@hi019
Copy link
Contributor

hi019 commented Oct 11, 2021

What about just accepting an S3 connection for the config?

@pcen
Copy link
Contributor Author

pcen commented Oct 11, 2021

What about just accepting an S3 connection for the config?

My rationale for having our own config is that the storage implementation's interface / config should not depend on an S3 struct, so if S3 implementation were to change then fiber's S3 storage implementation can change without breaking the interface to initialise it

hi019
hi019 suggested changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@hi019 hi019 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

s3/s3.go Outdated Show resolved Hide resolved
s3/s3.go Outdated Show resolved Hide resolved
@tomaswarynyca
Copy link

@pcen any news about finishing this?

@pcen
Copy link
Contributor Author

pcen commented Jan 20, 2022

@pcen any news about finishing this?

The code functionally all works, @hi019 requested changes for some small code cleanup, otherwise all that's left is to somehow test with the GitHub CI, I haven't had time to look into how to do it, but if someone else figures it out before I have time to it's ready to merge

@efectn
Copy link
Member

efectn commented Feb 2, 2022

I need s3 in one of my projects and this storage can ease my work. So i'll try to finish this PR.

Btw i'm not sure about SDK version. What about using v2? @pcen

@pcen
Copy link
Contributor Author

pcen commented Feb 2, 2022

I need s3 in one of my projects and this storage can ease my work. So i'll try to finish this PR.

Btw i'm not sure about SDK version. What about using v2? @pcen

When I started, I don't think v2 was available. Looking at the Amazon SDK repo for golang, it looks like the latest release is on v1.42.45. If you use the latest release version, it shouldn't cause any issues. If there is a newer version of the SDK, until it is implemented in golang, it won't effect us.

@efectn
Copy link
Member

efectn commented Feb 2, 2022

I need s3 in one of my projects and this storage can ease my work. So i'll try to finish this PR.
Btw i'm not sure about SDK version. What about using v2? @pcen

When I started, I don't think v2 was available. Looking at the Amazon SDK repo for golang, it looks like the latest release is on v1.42.45. If you use the latest release version, it shouldn't cause any issues. If there is a newer version of the SDK, until it is implemented in golang, it won't effect us.

Amazon has repo for go v2 sdk (https://github.com/aws/aws-sdk-go-v2)

Btw we need to enter env variables(creditianals) for exexuting tests. I think we don't need to add tests because of that. Like https://github.com/gofiber/storage/blob/main/dynamodb/dynamodb_test.go

@ReneWerner87 @hi019 What do you think?

@pcen
Copy link
Contributor Author

pcen commented Feb 2, 2022

I need s3 in one of my projects and this storage can ease my work. So i'll try to finish this PR.
Btw i'm not sure about SDK version. What about using v2? @pcen

When I started, I don't think v2 was available. Looking at the Amazon SDK repo for golang, it looks like the latest release is on v1.42.45. If you use the latest release version, it shouldn't cause any issues. If there is a newer version of the SDK, until it is implemented in golang, it won't effect us.

Amazon has repo for go v2 sdk (https://github.com/aws/aws-sdk-go-v2)

Btw we need to enter env variables(creditianals) for exexuting tests. I think we don't need to add tests because of that. Like https://github.com/gofiber/storage/blob/main/dynamodb/dynamodb_test.go

@ReneWerner87 @hi019 What do you think?

Had no idea about v2! in which case, we should definitely be using it!

@efectn efectn requested review from hi019 and ReneWerner87 February 3, 2022 12:06
ReneWerner87
ReneWerner87 requested changes Feb 3, 2022
View reviewed changes
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

please check all workflows, think something needs to be adjusted there, add the new storage to the readme in the root, so this is obvious as storage variant

and the unittests are still insufficient, they need to be extended, so we can deliver the quality the users are expecting from us

efectn added 3 commits February 3, 2022 19:51
@efectn
Add tests for S3
0aaa6d1
@efectn
update
ad7a9ed 
update

update

update

update

update

update

update

update

update

update

update

update

update
@efectn
update
9383752
hi019
hi019 suggested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@hi019 hi019 left a comment

Choose a reason for hiding this comment

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

Thanks for finishing this 👍

s3/README.md Outdated Show resolved Hide resolved
s3/s3.go Outdated Show resolved Hide resolved
s3/s3.go Outdated Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit 45cc6b9 into gofiber:main Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hi019 hi019 hi019 requested changes

@ReneWerner87 ReneWerner87 ReneWerner87 approved these changes

Assignees
No one assigned
Labels
✏️ Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pcen @hi019 @tomaswarynyca @efectn @ReneWerner87