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

Fix the v78 migration "Drop is_bare" on MSSQL #6707 #6823

Merged

Conversation

@zeripath
Copy link
Contributor

commented May 1, 2019

So #6707 appears to be reporting multiple issues with the drop bare constraint on MSSQL.

#6707 (comment)

suggests a piece of code which will try to detect the constraint and execute code to remove it.

This attempts to replicate this code.

Please note I do not have access to MSSQL - so this is untested. (Hence the WIP)

@codecov-io

This comment has been minimized.

Copy link

commented May 1, 2019

Codecov Report

Merging #6823 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6823      +/-   ##
==========================================
- Coverage   41.17%   41.17%   -0.01%     
==========================================
  Files         425      425              
  Lines       58477    58484       +7     
==========================================
+ Hits        24077    24078       +1     
- Misses      31215    31221       +6     
  Partials     3185     3185
Impacted Files Coverage Δ
models/migrations/v78.go 0% <0%> (ø) ⬆️
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
routers/repo/view.go 43.03% <0%> (+1.01%) ⬆️

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 a2a006a...0e89c08. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label May 1, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@zeripath we have a drone test about MSSQL. And you can run a mssql database on docker.

@zeripath zeripath changed the title WIP: Attempt to fix #6707 Fix the v78 migration "Drop is_bare" on MSSQL #6707 May 2, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Fixed and confirmed to be working!

@zeripath zeripath referenced this pull request May 2, 2019

Closed

Unable to migrate from version 1.7.4 to version 1.8.0 #6707

1 of 6 tasks complete
@lafriks
Copy link
Member

left a comment

Drop column is still beeded

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@lafriks what do you mean? The rest of the migrations don't explicitly drop the column - I think the sync2 does that?

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@zeripath

This comment has been minimized.

@lafriks

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

OK @lafriks I've changed to use dropTableColumns

@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 4, 2019

dropTableColumns drops columns but only removes the constraints from MSSQL. It misses indexes on other DB types.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Oh great so should we not update dropTableColumns to remove constraints on other types too?

@zeripath zeripath force-pushed the zeripath:fix-#6707-drop-is-bare-mssql branch from 2d4208a to a10debc May 4, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

I've reverted the change to dropTableColumns as it's not quite the right thing at present.

@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 4, 2019

This migration has caused many issues to be reported, I'd like to have it in a working state, then we can move the logic of dropping constraints into dropColumns.

On a related note: I wonder why our migration tests didn't catch the fail case. I'm guessing it's like that one user reported where they were using an older version of MSSQL.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

So we don't have migration tests for MSSQL as when I wrote the migration tests I didn't have access to MSSQL and it's not entirely clear how to get the T-SQL out of MSSQL.

If I knew XORM better I'd say it should be possible to get XORM to just dummy out the sql commands it would use to create and fill the database and we should be able to just store those but I don't know XORM well enough to do that.

@lunny

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@zeripath so, we need a migration test for MSSQL.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Yes we do.

The correct way to do this, and one that would make it very easy in future to add more migrations is for xorm dump to work correctly - but it doesn't produce valid sql.

Another way would be to somehow get xorm to rerun queries from its log. Then we could just store the log - and get xorm to run that. But I can't.

The way I made the migration tests in the first case was by breaking in the first integration test and then using that database's way of creating an SQL dump for it. It was extremely fiddly and MSSQL doesn't provide a way of doing that - although there are external things they do this. Xorm dump doesn't produce valid t-sql even when dumping from MSSQL.

I have to say it was not automatic to generate dumps postgres or mysql, and that their dumps required manual changes too to make them work. It's a highly unsatisfactory and manual way of making migrations. If xorm dump just worked correctly it would be so much easier.

@zeripath zeripath closed this May 5, 2019

@zeripath zeripath reopened this May 5, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Anyway there are multiple users out there who are struggling to get to 1.8 because this migration step is still wrong.

If you know a way of getting a t-sql dump out of mssql or can provide a t-sql dump of 1.5.3, 1.6.4 and 1.7.0 I'm happy to add this or we can add it as another pr - however we need to get a fix in.

Similarly if you'd prefer that this was rebased on top of #6849 that's fine, but #6849 would also need to be backported.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Build https://drone.gitea.io/go-gitea/gitea/8698/1/16 shows that when combined with #6851 and #6852 you can see fixes the v78 migration. I'm going to force rebase this back to be just the v78 fix now.

@zeripath zeripath force-pushed the zeripath:fix-#6707-drop-is-bare-mssql branch from c0a1c8a to 3b849c3 May 5, 2019

zeripath added some commits May 5, 2019

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels May 5, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 5, 2019

zeripath and others added some commits May 5, 2019

@techknowlogick techknowlogick merged commit d1a32fa into go-gitea:master May 5, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@zeripath merged. Please backport 😃

zeripath added a commit to zeripath/gitea that referenced this pull request May 5, 2019

@zeripath zeripath referenced this pull request May 5, 2019

Merged

Fix v78 migration (#6823) #6854

@zeripath zeripath deleted the zeripath:fix-#6707-drop-is-bare-mssql branch May 5, 2019

lunny added a commit that referenced this pull request May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.