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

Alter issue/comment table TEXT fields to LONGTEXT #16765

Merged
merged 4 commits into from
Aug 22, 2021

Conversation

wxiaoguang
Copy link
Contributor

The content field in issue, the content and patch field in comment are all TEXT type.

When using MySQL, TEXT type has a limitation of 64KB chars. It will case failure when migrating a repository containing an issue with very long content.

And, no other database has such limitation as MySQL. So it's good to make issue system's behavior the same with different databases.

Issue: #16656

@wxiaoguang
Copy link
Contributor Author

The CI failure is caused by TestRepository_GetTag, it seems that it's not related to this PR ......

2021/08/21 14:38:37 ...dules/git/command.go:143:RunWithContext() [D] /drone/src/modules/git/tests/repos/repo1_TestRepository_GetTag: /usr/bin/git -c credential.helper= -c protocol.version=2 show-ref --tags -- lightweightTag
210s
2211 --- FAIL: TestRepository_GetTag (0.21s)
210s
2212 panic: runtime error: invalid memory address or nil pointer dereference [recovered]
210s
2213	panic: runtime error: invalid memory address or nil pointer dereference
210s
2214 [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xac2d5d]
210s
2215
210s
2216 goroutine 1964 [running]:
210s
2217 testing.tRunner.func1.2({0xc170a0, 0x1dd90c0})
210s
2218	/usr/local/go/src/testing/testing.go:1209 +0x36c
210s
2219 testing.tRunner.func1()
210s
2220	/usr/local/go/src/testing/testing.go:1212 +0x3b6
210s
2221 panic({0xc170a0, 0x1dd90c0})
210s
2222	/usr/local/go/src/runtime/panic.go:1047 +0x266
210s
2223 code.gitea.io/gitea/modules/git.TestRepository_GetTag(0x0)
210s
2224	/drone/src/modules/git/repo_tag_test.go:52 +0x29d
210s
2225 testing.tRunner(0xc0016801a0, 0xdcf750)
210s
2226	/usr/local/go/src/testing/testing.go:1259 +0x230
210s
2227 created by testing.(*T).Run
210s
2228	/usr/local/go/src/testing/testing.go:1306 +0x727
210s
2229 FAIL	code.gitea.io/gitea/modules/git	6.324s

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2021

Codecov Report

Merging #16765 (5bd244c) into main (7f85610) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16765      +/-   ##
==========================================
- Coverage   45.50%   45.49%   -0.02%     
==========================================
  Files         762      762              
  Lines       85939    85939              
==========================================
- Hits        39108    39098      -10     
- Misses      40519    40527       +8     
- Partials     6312     6314       +2     
Impacted Files Coverage Δ
models/issue.go 58.52% <ø> (ø)
models/issue_comment.go 52.05% <ø> (+0.29%) ⬆️
modules/notification/mail/mail.go 35.29% <0.00%> (-2.95%) ⬇️
models/unit.go 41.09% <0.00%> (-2.74%) ⬇️
modules/process/manager.go 72.83% <0.00%> (-2.47%) ⬇️
modules/log/event.go 60.64% <0.00%> (-1.86%) ⬇️
modules/notification/ui/ui.go 60.86% <0.00%> (-1.45%) ⬇️
models/notification.go 65.13% <0.00%> (-0.88%) ⬇️
services/pull/pull.go 41.78% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f85610...5bd244c. Read the comment docs.

models/migrations/v191.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Small nit but otherwise LGTM

@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 21, 2021
@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 Aug 22, 2021
@wxiaoguang wxiaoguang closed this Aug 22, 2021
@wxiaoguang wxiaoguang deleted the alter-issue-content-longtext branch August 22, 2021 05:12
@zeripath

This comment was marked as off-topic.

@wxiaoguang wxiaoguang restored the alter-issue-content-longtext branch August 22, 2021 07:05
@wxiaoguang wxiaoguang reopened this Aug 22, 2021
@wxiaoguang

This comment was marked as off-topic.

@lunny lunny merged commit b55c699 into go-gitea:main Aug 22, 2021
@wxiaoguang wxiaoguang deleted the alter-issue-content-longtext branch October 12, 2021 04:46
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
@wxiaoguang wxiaoguang added this to the 1.16.0 milestone Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants