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

AmazonS3: allow not implemented versioning #31498

Merged
merged 1 commit into from
Mar 15, 2022
Merged

AmazonS3: allow not implemented versioning #31498

merged 1 commit into from
Mar 15, 2022

Conversation

Erikvv
Copy link
Contributor

@Erikvv Erikvv commented Mar 9, 2022

In case the S3 implementation does not implement versioning, set it to
false. Versioning was introduced in Nexcloud in commit
09ffac5

This is needed for compatibility with the Storj gateway.

@szaimen szaimen added this to the Nextcloud 24 milestone Mar 9, 2022
@szaimen szaimen added the 3. to review Waiting for reviews label Mar 9, 2022
@szaimen szaimen requested a review from icewind1991 March 9, 2022 11:01
@Erikvv
Copy link
Contributor Author

Erikvv commented Mar 9, 2022

We're debating whether to change this on Storj side instead.

@Erikvv
Copy link
Contributor Author

Erikvv commented Mar 9, 2022

We would like for you to consider this change.

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

The code looks good, but I don't have a Storj gateway to test it

@Erikvv
Copy link
Contributor Author

Erikvv commented Mar 11, 2022

The code looks good, but I don't have a Storj gateway to test it

You can sign up for free and get credentials for the Storj-hosted gateway.

Alternatively I can DM you credentials.

We also have a few ways to set up a local independent network but that is a bit more involved. One option is https://github.com/storj/up, includes gateway-mt (multitent) but not gateway-st which you could add from https://hub.docker.com/r/storjlabs/gateway. Other option is https://github.com/storj/storj/wiki/Test-network, maybe easier.

@artonge
Copy link
Contributor

artonge commented Mar 14, 2022

Code looks good, but I did not test it. It only adds a try/catch, so it should be safe to merge.

@Erikvv I assume that you tested it on your server with your storj gateway?

@Erikvv
Copy link
Contributor Author

Erikvv commented Mar 14, 2022

@Erikvv I assume that you tested it on your server with your storj gateway?

I have tested it with storj/gateway-st and I'm now able to list and download objects.

On upload there is the error #26544 but I haven't looked into this further. I know that it worked last year though.

@Erikvv
Copy link
Contributor Author

Erikvv commented Mar 14, 2022

S3 test failed but error seems unrelated, S3 server seemed down.

In case the S3 implementation does not implement versioning, set it to
false. Versioning was introduced in Nexcloud in commit
09ffac5

This is needed for compatibility with the Storj gateway.

Signed-off-by: Erik van Velzen <erik@evanv.nl>
@artonge
Copy link
Contributor

artonge commented Mar 15, 2022

All green, merging :)

@artonge artonge merged commit 1ae6b2b into nextcloud:master Mar 15, 2022
@welcome
Copy link

welcome bot commented Mar 15, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@artonge
Copy link
Contributor

artonge commented Mar 15, 2022

Thanks for this @Erikvv ! :)

@icewind1991
Copy link
Member

/backport to stable23

@icewind1991
Copy link
Member

/backport to stable22

@icewind1991
Copy link
Member

/backport to stable21

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

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

Successfully merging this pull request may close these issues.

6 participants