Skip to content

Fix broken v27 migration - change mirror interval from int to bigint - #1504

Merged
lunny merged 1 commit into
go-gitea:masterfrom
cez81:mirror_interval_bigint
Apr 30, 2017
Merged

Fix broken v27 migration - change mirror interval from int to bigint#1504
lunny merged 1 commit into
go-gitea:masterfrom
cez81:mirror_interval_bigint

Conversation

@cez81

@cez81 cez81 commented Apr 17, 2017

Copy link
Copy Markdown
Contributor

Fix for #1496. Migration to v27 didn't alter the column type to bigint (#1407). This fix will alter the column type and set all mirror intervals that are < Mirror.MinInterval to Mirror.DefaultInterval.
Sqlite is ok because uses same integer type.

Comment thread models/migrations/v29.go Outdated

@cez81 cez81 Apr 17, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This might be better?
if engine.Dialect().DataSourceName() == "mysql" { engine.Exec("ALTER TABLE mirror MODIFY interval BIGINT"); }

But I guess the same fix need to be applied for Postgresql as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed in Postgres #1505

Comment thread models/migrations/v29.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing mssql

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

Comment thread models/migrations/v29.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not use sess.Exec since we have in a transaction?

@lunny lunny added this to the 1.2.0 milestone Apr 18, 2017
@lunny lunny added the type/bug label Apr 18, 2017
@lunny

lunny commented Apr 21, 2017

Copy link
Copy Markdown
Member

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 21, 2017
@sapk

sapk commented Apr 21, 2017

Copy link
Copy Markdown
Member

Doesn't this fix need also change in models/migrations/v27.go ? Since the failed migration will still occur on applying v27 before this migration ?

@sapk

sapk commented Apr 21, 2017

Copy link
Copy Markdown
Member

In fact all this change should be done in v27 since normally failing migration would stop the migration and people would be in blocked in v26 ?

@lunny

lunny commented Apr 21, 2017

Copy link
Copy Markdown
Member

According to #1496, there is no error on migration v27.

@lunny

lunny commented Apr 21, 2017

Copy link
Copy Markdown
Member

Yes. I'm wrong. @cez81 You are right. You should change the migration v27 but not add a new one.

@cez81 cez81 changed the title Migration to change mirror interval from int to bigint Fix broken v27 migration - change mirror interval from int to bigint Apr 21, 2017
Comment thread models/migrations/v27.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are 5 fields being dropped here from the Mirror struct ? Is this intentional ? Would it affect database structure ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, just don't need them during the migration. As I understand it Xorm will notice and warn but fields are still there. Same as repo struct above. It has more fields not listed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

^

Comment thread models/migrations/v27.go Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I dunno if XORM takes care of this @lunny but PostgreSQL quotes for identifiers should be doublequote, not backtick.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I haven't used PostgreSQL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks like SQL Server also uses doublequotes.

Mirror.interval column type needed to be changed to bigint.
Correct interval where the interval set is < MinInterval.
@cez81
cez81 force-pushed the mirror_interval_bigint branch from 62214aa to 9994626 Compare April 25, 2017 21:30
@cez81

cez81 commented Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Rebased to pass Drone build

@bkcsoft

bkcsoft commented Apr 29, 2017

Copy link
Copy Markdown
Contributor

LGTM

@tboerger tboerger 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 Apr 29, 2017
@lunny
lunny merged commit 9a8ca3e into go-gitea:master Apr 30, 2017
@cez81
cez81 deleted the mirror_interval_bigint branch May 2, 2017 10:50
@Morlinest Morlinest mentioned this pull request Aug 14, 2017
7 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants