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.

Sign up for GitHub

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

Jump to bottom

Fix gpg wrong column types #1303

Merged
merged 1 commit into from
Mar 20, 2017
Merged

Fix gpg wrong column types #1303

merged 1 commit into from
Mar 20, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 17, 2017

This will fix #1283 @sapk could you confirm this will not break GPG feature?

@lunny lunny added the type/bug label Mar 17, 2017
@lunny lunny added this to the 1.2.0 milestone Mar 17, 2017
@sapk
Copy link
Member

sapk commented Mar 17, 2017 •
edited
Loading

This should be ok but I will test it in 4 hour after work.

The migration is a little hard but we don't care at this stage ^^

The test build seem broken ...

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 17, 2017
@sapk
Copy link
Member

sapk commented Mar 17, 2017 •
edited
Loading

API work perfect but can't confirm if it fix mariadb compat prob right now. I trust you on that ^^

@fabian-z
Copy link
Contributor

This fixes the issue for me on MariaDB 10.1.21

@strk
Copy link
Member

strk commented Mar 18, 2017

Trusted LGTM

@tboerger tboerger 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 Mar 18, 2017
@strk
Copy link
Member

strk commented Mar 18, 2017

Trusted LGTM

@shyim shyim mentioned this pull request Mar 19, 2017
Closed
7 tasks
bkcsoft
bkcsoft suggested changes Mar 19, 2017
View reviewed changes
models/migrations/v24.go
CanCertify bool
}

if err := x.DropTables(new(GPGKey)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

So you're dropping all added GPG-keys? 😱

Copy link
Member

Choose a reason for hiding this comment

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

For now they shouldn't have any key in most db since the only available method is through the API.

Copy link
Member

Choose a reason for hiding this comment

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

Then why migrate at all ?

Copy link
Member

Choose a reason for hiding this comment

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

Because if someone allready upgrade to master (with a other backend than mariadb) he will not have the same table that the one that don't ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like @sapk said, no UI to do that and no body could really add a gpgkey since #1283 will refuse that. And change a column is so different between different database that drop a table and create it will a better method.

Copy link
Member

Choose a reason for hiding this comment

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

@lunny Might be a good idea to add that to the description next time 😉

@sapk sapk mentioned this pull request Mar 19, 2017
Merged
6 tasks
@bkcsoft
Copy link
Member

bkcsoft commented Mar 20, 2017

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 Mar 20, 2017
@bkcsoft bkcsoft merged commit 608cd54 into go-gitea:master Mar 20, 2017
@lunny lunny deleted the lunny/fix_gpg_key_error branch March 20, 2017 02:20
@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.
Reviewers

@strk strk strk left review comments

@sapk sapk sapk left review comments

@bkcsoft bkcsoft bkcsoft approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

MYSQL: Database error on installation
6 participants
@lunny @sapk @fabian-z @strk @bkcsoft @tboerger