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

Storage configuration support [storage] #13314

Merged
merged 5 commits into from
Oct 31, 2020

Conversation

lunny
Copy link
Member

@lunny lunny commented Oct 26, 2020

This PR adjusted the override sequence, [storage] <- [storage.minio] <- [storage.attachments]. Right section will override left.

In fact, [storage] is expected because it's more easier to remember.

This will also enable storage tests with minio when mysql integration tests.

This also changed the behaviour of lfs serve when given a range start value out of bounds, now it will return 416, but not ignore that like before.

@lunny lunny added type/enhancement An improvement of existing functionality backport/v1.13 labels Oct 26, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 26, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 26, 2020
@lunny lunny force-pushed the lunny/fix_minio branch 2 times, most recently from f86ca3a to 8c5bec1 Compare October 29, 2020 02:27
@techknowlogick
Copy link
Member

TestGetLFSRange test fail with minio

@lunny
Copy link
Member Author

lunny commented Oct 31, 2020

The file tested in LFS is 10 bytes, and the request is range=11-, so the correct response of HTTP standard is to return an error or just ignore that?

The behaviour between os.File's Seek and minio's Seek is different when seek over file's size. os.File will always return no error but minio will return error with io.EOF.

@lafriks
Copy link
Member

lafriks commented Oct 31, 2020

It should be fixed for file and minio to behave the same and handle that

@lunny
Copy link
Member Author

lunny commented Oct 31, 2020

@lafriks Yes, so which one should choose? Return io.EOF or just ignore them?

@lunny
Copy link
Member Author

lunny commented Oct 31, 2020

OK. I think https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests explained we should return 416.

Partial request responses

There are three relevant statuses, when working with range requests:

    In case of a successful range request, the 206 Partial Content status is sent back from a server.
    In case of a range request that is out of bounds (none of the range values overlap the extent of the resource, i.e first-byte-pos of all ranges is greater than the resource length), the server responds with a 416 Requested Range Not Satisfiable status.
    In case of no support of range requests, the 200 OK status is sent back from a server.

@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit e4e85a3 into go-gitea:master Oct 31, 2020
@techknowlogick
Copy link
Member

@lunny please send backport :)

@lunny lunny deleted the lunny/fix_minio branch November 1, 2020 01:19
lunny added a commit to lunny/gitea that referenced this pull request Nov 1, 2020
* Fix minio bug

* Add tests for storage configuration

* Change the Seek flag to keep compitable minio?

* Fix test when first-byte-pos of all ranges is greater than the resource length

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@lunny lunny added the backport/done All backports for this PR have been created label Nov 1, 2020
lunny added a commit that referenced this pull request Nov 1, 2020
* Fix minio bug

* Add tests for storage configuration

* Change the Seek flag to keep compitable minio?

* Fix test when first-byte-pos of all ranges is greater than the resource length

Co-authored-by: techknowlogick <techknowlogick@gitea.io>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants