Fix merging artifact chunks error when minio storage basepath is set #28555
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
github-actions
bot
added
the
modifies/api
wxiaoguang
reviewed
Dec 20, 2023
lunny
reviewed
Dec 20, 2023
delvh
approved these changes
Dec 20, 2023
GiteaBot
added
lgtm/need 1
fuxiaohei
changed the title
wxiaoguang
reviewed
Dec 20, 2023
| baseName := filepath.Base(fpath) | ||
| // when read chunks from storage, it only contains storage dir and basename, | ||
| // no matter the subdirectory setting in storage config | ||
| item := chunkFileItem{Path: path.Join(storageDir, baseName)} |
There was a problem hiding this comment.
It confused me that it sometimes calls filepath (line 74) while sometimes calls path (here). It looks quite fragile to me.
At least, IMO it needs to normalize the "paths" before using it.
There was a problem hiding this comment.
The second path.Join is not necessary. It saves chunk as filename tmp/xxx.chunk in above. So just storageDir +"/" + basename is enough.
There was a problem hiding this comment.
I still have difficulty to understand whether it is right to do these magic path operations.
Could it clarify these "path kinds"? If a path is a filesystem path, then it needs filepath package, if it is a web path (or a web path converted from filepath), then it needs util.PathJoinRelX.
There was a problem hiding this comment.
baseName := filepath.Base(fpath) is the real fix. fpath is from Storage and can be filesystem path so use filepath to get basename.
chunkFileItem.Path is a key to read and write file for Storage. When saving chunk, it uses fmt.Sprintf("tmp%d/%d-%d-%d-%d.chunk", runID, runID, artifact.ID, start, end) with / slash when saving into Storage.
Using storageDir +"/" + basename is enough in item.Path to read from Storage. Storage module can handle os difference.
fuxiaohei
force-pushed
the
fix/artifact-upload-minio
branch
from
a3f0872
to
3644975
Compare
lunny
approved these changes
Dec 21, 2023
GiteaBot
added
lgtm/done
lunny
added
reviewed/wait-merge
GiteaBot
pushed a commit
to GiteaBot/gitea
that referenced
this pull request
Dec 21, 2023
…o-gitea#28555) Related to go-gitea#28279 When merging artifact chunks, it lists chunks from storage. When storage is minio, chunk's path contains `MINIO_BASE_PATH` that makes merging break. <del>So trim the `MINIO_BASE_PATH` when handle chunks.</del> Update the chunk file's basename to retain necessary information. It ensures that the directory in the chunk's path remains unaffected.
GiteaBot
added
backport/done
lunny
pushed a commit
that referenced
this pull request
Dec 21, 2023
…28555) (#28568) Backport #28555 by @fuxiaohei Related to #28279 When merging artifact chunks, it lists chunks from storage. When storage is minio, chunk's path contains `MINIO_BASE_PATH` that makes merging break. <del>So trim the `MINIO_BASE_PATH` when handle chunks.</del> Update the chunk file's basename to retain necessary information. It ensures that the directory in the chunk's path remains unaffected. Co-authored-by: FuXiaoHei <fuxiaohei@vip.qq.com>
zjjhot
added a commit
to zjjhot/gitea
that referenced
this pull request
Dec 22, 2023
* giteaofficial/main: Add more ways to try (go-gitea#28581) Convert to url auth to header auth in tests (go-gitea#28484) Fix 500 error of searching commits (go-gitea#28576) improve possible performance bottleneck (go-gitea#28547) Use information from previous blame parts (go-gitea#28572) Make offline mode as default to no connect external avatar service by default (go-gitea#28548) Fix merging artifact chunks error when minio storage basepath is set (go-gitea#28555) feat: bump `dessant/lock-threads` and `actions/setup-go` to use nodejs20 runtime (go-gitea#28565) Update actions document about comparsion as Github Actions (go-gitea#28560) Fix inperformant query on retrifing review from database. (go-gitea#28552) Fix the issue ref rendering for wiki (go-gitea#28556) Add missing head of lfs client batch (go-gitea#28550) [skip ci] Updated translations via Crowdin Remove deadcode under models/issues (go-gitea#28536) Always enable caches (go-gitea#28527) Improve ObjectFormat interface (go-gitea#28496)
fuxiaohei
added a commit
to fuxiaohei/gitea
that referenced
this pull request
Jan 17, 2024
…o-gitea#28555) Related to go-gitea#28279 When merging artifact chunks, it lists chunks from storage. When storage is minio, chunk's path contains `MINIO_BASE_PATH` that makes merging break. <del>So trim the `MINIO_BASE_PATH` when handle chunks.</del> Update the chunk file's basename to retain necessary information. It ensures that the directory in the chunk's path remains unaffected.
silverwind
pushed a commit
to silverwind/gitea
that referenced
this pull request
Feb 20, 2024
…o-gitea#28555) Related to go-gitea#28279 When merging artifact chunks, it lists chunks from storage. When storage is minio, chunk's path contains `MINIO_BASE_PATH` that makes merging break. <del>So trim the `MINIO_BASE_PATH` when handle chunks.</del> Update the chunk file's basename to retain necessary information. It ensures that the directory in the chunk's path remains unaffected.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #28279
When merging artifact chunks, it lists chunks from storage. When storage is minio, chunk's path contains
MINIO_BASE_PATHthat makes merging break.So trim theMINIO_BASE_PATHwhen handle chunks.Update the chunk file's basename to retain necessary information. It ensures that the directory in the chunk's path remains unaffected.