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

Supports forced use of S3 virtual-hosted style #30969

Merged
merged 15 commits into from
May 15, 2024
Merged

Conversation

dicarne
Copy link
Contributor

@dicarne dicarne commented May 14, 2024

Add a configuration item to enable S3 virtual-hosted style (V2) to solve the problem caused by some S3 service providers not supporting path style (V1).

Resolves #14991

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 14, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label May 14, 2024
Copy link
Member

@techknowlogick techknowlogick 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 this PR! Could you also add the new config option to the docs (and app.ini example)? I'm on mobile, but if you need help finding where to add documentation for this please let me know and I can send a link when I'm at my computer:)

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 14, 2024
@dicarne
Copy link
Contributor Author

dicarne commented May 14, 2024

Thanks for this PR! Could you also add the new config option to the docs (and app.ini example)? I'm on mobile, but if you need help finding where to add documentation for this please let me know and I can send a link when I'm at my computer:)

Okay, I added it to the documentation.

@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 May 14, 2024
docs/content/administration/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved
modules/setting/storage.go Outdated Show resolved Hide resolved
Copy link
Member

@jolheiser jolheiser 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 PR!

@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 May 14, 2024
@silverwind
Copy link
Member

Unit test failure looks related:

--- FAIL: TestMinioStorageIterator (0.00s)
    storage_test.go:17: 
        	Error Trace:	/home/runner/work/gitea/gitea/modules/storage/storage_test.go:17
        	            				/home/runner/work/gitea/gitea/modules/storage/minio_test.go:23
        	Error:      	Received unexpected error:
        	            	invalid minio bucket lookup type: 
        	Test:       	TestMinioStorageIterator
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xa1a441]

@jolheiser
Copy link
Member

Ah, yeah that's my bad. getDefaultStorageSection doesn't do what I thought it did. It looks like instead it may be inside getStorageForMinio

@dicarne
Copy link
Contributor Author

dicarne commented May 15, 2024

testStorageIterator(t, setting.MinioStorageType, &setting.Storage{
MinioConfig: setting.MinioStorageConfig{
Endpoint: "127.0.0.1:9000",
AccessKeyID: "123456",
SecretAccessKey: "12345678",
Bucket: "gitea",
Location: "us-east-1",
},

It seems that the unit test directly assigns values to the minio configuration without going through the logic of assigning default values.

@github-actions github-actions bot added the modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin label May 15, 2024
@KN4CK3R KN4CK3R added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 15, 2024
@KN4CK3R KN4CK3R added this to the 1.23.0 milestone May 15, 2024
@jolheiser jolheiser merged commit d0d6aad into go-gitea:main May 15, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 15, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 16, 2024
* giteaofficial/main:
  Put web editor into a segment (go-gitea#30966)
  Check if the release is converted from the tag when updating the release (go-gitea#30984)
  Supports forced use of S3 virtual-hosted style (go-gitea#30969)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/docs modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support S3 non-pathstyle (V2)
7 participants