Skip to content

MinIO > 2021 interprets a / prefix differently #26841

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

Closed
wants to merge 1 commit into from

Conversation

earl-warren
Copy link
Contributor

@earl-warren earl-warren commented Aug 31, 2023

$ minio --version
minio version RELEASE.2023-08-23T10-07-06Z (commit-id=af564b8ba07a7805a578b5f6f2b3827490f74ccd)
$ make CI=true GOTESTFLAGS='-v -count=1' GO_TEST_PACKAGES=code.gitea.io/gitea/modules/storage FLAGS= 'test#TestMinioStorageIterator'
Running go test with -tags 'sqlite sqlite_unlock_notify'...
=== RUN   TestMinioStorageIterator
2023/08/31 14:09:28 ...les/storage/minio.go:81:NewMinioStorage() [I] Creating Minio storage at 127.0.0.1:9000:gitea with base path
    storage_test.go:49:
        	Error Trace:	/home/earl-warren/software/forgejo/modules/storage/storage_test.go:49
        	            				/home/earl-warren/software/forgejo/modules/storage/minio_test.go:18
        	Error:      	"[a/1.txt b/1.txt b/2.txt b/3.txt b/x 4.txt ab/1.txt]" should have 0 item(s), but has 6
        	Test:       	TestMinioStorageIterator
--- FAIL: TestMinioStorageIterator (0.30s)
FAIL
FAIL	code.gitea.io/gitea/modules/storage	0.306s
FAIL
make: *** [Makefile:470: test#TestMinioStorageIterator] Error 1

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 31, 2023
@puni9869
Copy link
Member

Can you add a test case around it, if applicable.

@silverwind
Copy link
Member

Does this mean we require a certain minimum version of minio or is it backwards-compatible?

@earl-warren
Copy link
Contributor Author

The existing tests fail when used with a recent MinIO server: this part of the code is covered.

@silverwind
Copy link
Member

I see we are testing with bitnami/minio:2021.3.17, so I guess keep it at that version.

image: bitnami/minio:2021.3.17

@wxiaoguang
Copy link
Contributor

The logic is still not right. For example, basePath may not have the trailing slash.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 31, 2023
@silverwind silverwind self-requested a review August 31, 2023 21:05
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 31, 2023
@earl-warren
Copy link
Contributor Author

The logic is still not right. For example, basePath may not have the trailing slash.

Do you mean the logic of this fix is not right? Or that the logic of the minio implementation is not right?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 11, 2023

I guess "the logic of this fix is not right".

In most Object Storages, prefix "dir" matchs "dir-1" and "dir/sub". If it needs to only match "dir/sub", it needs to use prefix "dir/" to list.

$ minio --version
minio version RELEASE.2023-08-23T10-07-06Z (commit-id=af564b8ba07a7805a578b5f6f2b3827490f74ccd)

Running go test with -tags 'sqlite sqlite_unlock_notify'...
=== RUN   TestMinioStorageIterator
2023/08/31 14:09:28 ...les/storage/minio.go:81:NewMinioStorage() [I] Creating Minio storage at 127.0.0.1:9000:gitea with base path
    storage_test.go:49:
        	Error Trace:	/home/earl-warren/software/forgejo/modules/storage/storage_test.go:49
        	            				/home/earl-warren/software/forgejo/modules/storage/minio_test.go:18
        	Error:      	"[a/1.txt b/1.txt b/2.txt b/3.txt b/x 4.txt ab/1.txt]" should have 0 item(s), but has 6
        	Test:       	TestMinioStorageIterator
--- FAIL: TestMinioStorageIterator (0.30s)
FAIL
FAIL	code.gitea.io/gitea/modules/storage	0.306s
FAIL
make: *** [Makefile:470: test#TestMinioStorageIterator] Error 1
@wxiaoguang
Copy link
Contributor

I guess the right fix is like this: Fix object storage path handling #27024

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants