Fix wrong type on hooktask to convert typ from char(16) to varchar(16) #14148

lunny · 2020-12-25T12:22:35Z

services/webhook/deliver.go

CirnoT

Please revert the other TrimSpace change that happened in Possible fix the webhook API creation #13960
Please ensure migration trims existing whitespaces or it won't fix existing webhooks

models/migrations/v163.go

lafriks · 2021-01-04T13:39:51Z

@lunny migration fails on MSSQL:

[I] Migration[165]: Convert hook task type from char(16) to varchar(16) and trim the column
migration_test.go:259:
            	Error Trace:	migration_test.go:259
            	            				migration_test.go:296
            	Error:      	Received unexpected error:
            	            	migrate: do migrate: mssql: Incorrect syntax near 'MODIFY'.

lunny · 2021-01-04T14:56:42Z

@lunny migration fails on MSSQL:

[I] Migration[165]: Convert hook task type from char(16) to varchar(16) and trim the column
migration_test.go:259:
            	Error Trace:	migration_test.go:259
            	            				migration_test.go:296
            	Error:      	Received unexpected error:
            	            	migrate: do migrate: mssql: Incorrect syntax near 'MODIFY'.

Yes, I think this should be a bug of xorm for mssql

zeripath · 2021-01-04T17:33:07Z

just use recreatetable ...

models/migrations/v165.go

lafriks · 2021-01-04T20:00:43Z

just use recreatetable ...

That could be expensive on servers with many webhook calls as I think webhook history table tends to be largest one of all 😅

lunny · 2021-01-05T02:31:28Z

depends on https://gitea.com/xorm/xorm/pulls/1849

lunny · 2021-01-05T03:34:44Z

blocked by #14246

lafriks · 2021-01-05T08:57:50Z

Looks like MSSQL needs separate session for alter table also

CirnoT · 2021-01-05T09:07:11Z

just use recreatetable ...

That could be expensive on servers with many webhook calls as I think webhook history table tends to be largest one of all 😅

Implying that this ALTER won't cause copy to tmp on MySQL anyway...

lunny · 2021-01-05T11:21:49Z

@CirnoT @lafriks @zeripath Please review again.

models/migrations/v165.go

…ld use it

models/migrations/migrations.go

models/migrations/v161.go

models/migrations/migrations.go

codecov-io · 2021-01-05T22:23:33Z

Codecov Report

Merging #14148 (b2de9dd) into master (d2ee122) will decrease coverage by 0.02%.
The diff coverage is 1.56%.

@@            Coverage Diff             @@
##           master   #14148      +/-   ##
==========================================
- Coverage   41.86%   41.84%   -0.03%     
==========================================
  Files         742      743       +1     
  Lines       79324    79384      +60     
==========================================
+ Hits        33209    33218       +9     
- Misses      40650    40704      +54     
+ Partials     5465     5462       -3

Impacted Files	Coverage Δ
models/migrations/migrations.go	`2.28% <0.00%> (-0.16%)`	⬇️
models/migrations/v161.go	`0.00% <0.00%> (ø)`
models/migrations/v165.go	`0.00% <0.00%> (ø)`
models/webhook.go	`69.06% <ø> (ø)`
services/webhook/webhook.go	`52.42% <33.33%> (ø)`
modules/util/timer.go	`42.85% <0.00%> (-42.86%)`	⬇️
modules/queue/unique_queue_disk_channel.go	`55.38% <0.00%> (+1.53%)`	⬆️
modules/charset/charset.go	`73.03% <0.00%> (+4.49%)`	⬆️
modules/git/repo_language_stats_nogogit.go	`63.82% <0.00%> (+6.38%)`	⬆️
... and 1 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 d2ee122...b2de9dd. Read the comment docs.

block reason was resolved - outdated

* master: (252 commits) Issues overview should not show issues from archived repos (go-gitea#13220) Display SVG files as images instead of text (go-gitea#14101) [skip ci] Updated translations via Crowdin Update docs to clarify issues raised in go-gitea#14272 (go-gitea#14318) [skip ci] Updated translations via Crowdin [Refactor] Passwort Hash/Set (go-gitea#14282) Add option to change username to the admin panel (go-gitea#14229) fix mailIssueCommentBatch for pull request (go-gitea#14252) Remove self from MAINTAINERS (go-gitea#14286) Do not reload page after adding comments in Pull Request reviews (go-gitea#13877) Fix session bug when introduce chi (go-gitea#14287) [skip ci] Updated translations via Crowdin Add secure/httpOnly attributes to the lang cookie (go-gitea#9690) (go-gitea#14279) Some code improvements (go-gitea#14266) [skip ci] Updated translations via Crowdin Fix wrong type on hooktask to convert typ from char(16) to varchar(16) (go-gitea#14148) Upgrade XORM links in documentation. (go-gitea#14265) Check permission for the appropriate unit type (go-gitea#14261) Add compliance check for windows to ensure cross platform build (go-gitea#14260) [skip ci] Updated translations via Crowdin ...

lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Dec 25, 2020

lunny force-pushed the lunny/fix_webhook branch from 24d7273 to 81dd755 Compare December 25, 2020 12:41

CirnoT reviewed Dec 26, 2020

View reviewed changes

services/webhook/deliver.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 26, 2020

CirnoT suggested changes Dec 26, 2020

View reviewed changes

zeripath previously requested changes Dec 26, 2020

View reviewed changes

models/migrations/v163.go Outdated Show resolved Hide resolved

lunny force-pushed the lunny/fix_webhook branch 5 times, most recently from eccafbd to 97d1a0a Compare January 1, 2021 15:40

lunny force-pushed the lunny/fix_webhook branch from 97d1a0a to 36caf76 Compare January 3, 2021 05:17

lafriks mentioned this pull request Jan 4, 2021

Merge default and system webhooks under one menu #14244

Merged

6543 reviewed Jan 4, 2021

View reviewed changes

models/migrations/v165.go Show resolved Hide resolved

lunny added 9 commits January 5, 2021 14:29

Fix wrong type on hooktask to convert typ from char(16) to varchar(16)

abd3ca8

Fix bugs

5cb1ec2

Improve code

19d2a6b

Use different trim function for MSSQL

6bb67aa

Fix bug

a895e03

Removed wrong changed line

ef2fb77

Removed wrong changed line

4516d70

Fix nullable

afebd2f

Fix lint

c30dbd8

Fix mssql modify column failure

1343067

lafriks reviewed Jan 5, 2021

View reviewed changes

models/migrations/v165.go Show resolved Hide resolved

lafriks reviewed Jan 5, 2021

View reviewed changes

models/migrations/v165.go Outdated Show resolved Hide resolved

Move modifyColumn to migrations.go so that other migrate function cou…

7fd394d

…ld use it

lunny force-pushed the lunny/fix_webhook branch from 34da08e to 7fd394d Compare January 5, 2021 12:30

lafriks approved these changes Jan 5, 2021

View reviewed changes

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 Jan 5, 2021

lafriks requested review from CirnoT and zeripath January 5, 2021 12:47

6543 reviewed Jan 5, 2021

View reviewed changes

models/migrations/migrations.go Show resolved Hide resolved

CirnoT reviewed Jan 5, 2021

View reviewed changes

models/migrations/v161.go Show resolved Hide resolved

CirnoT reviewed Jan 5, 2021

View reviewed changes

models/migrations/migrations.go Show resolved Hide resolved

6543 approved these changes Jan 5, 2021

View reviewed changes

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 Jan 5, 2021

Merge branch 'master' into lunny/fix_webhook

726b7e8

6543 requested a review from CirnoT January 5, 2021 21:55

CirnoT approved these changes Jan 6, 2021

View reviewed changes

Merge branch 'master' into lunny/fix_webhook

b2de9dd

6543 merged commit 9f73cae into go-gitea:master Jan 6, 2021

lunny deleted the lunny/fix_webhook branch January 7, 2021 01:49

go-gitea locked and limited conversation to collaborators Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong type on hooktask to convert typ from char(16) to varchar(16) #14148

Fix wrong type on hooktask to convert typ from char(16) to varchar(16) #14148

lunny commented Dec 25, 2020

CirnoT left a comment

lafriks commented Jan 4, 2021

lunny commented Jan 4, 2021

zeripath commented Jan 4, 2021

lafriks commented Jan 4, 2021

lunny commented Jan 5, 2021

lunny commented Jan 5, 2021

lafriks commented Jan 5, 2021

CirnoT commented Jan 5, 2021 •

edited

lunny commented Jan 5, 2021

codecov-io commented Jan 5, 2021 •

edited

Fix wrong type on hooktask to convert typ from char(16) to varchar(16) #14148

Fix wrong type on hooktask to convert typ from char(16) to varchar(16) #14148

Conversation

lunny commented Dec 25, 2020

CirnoT left a comment

Choose a reason for hiding this comment

lafriks commented Jan 4, 2021

lunny commented Jan 4, 2021

zeripath commented Jan 4, 2021

lafriks commented Jan 4, 2021

lunny commented Jan 5, 2021

lunny commented Jan 5, 2021

lafriks commented Jan 5, 2021

CirnoT commented Jan 5, 2021 • edited

lunny commented Jan 5, 2021

codecov-io commented Jan 5, 2021 • edited

Codecov Report

CirnoT commented Jan 5, 2021 •

edited

codecov-io commented Jan 5, 2021 •

edited