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

Check if column exist before rename if exist, just return with no error #17870

Merged
merged 4 commits into from
Dec 2, 2021
Merged

Check if column exist before rename if exist, just return with no error #17870

merged 4 commits into from
Dec 2, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 1, 2021 •
edited
Loading

Some people upgrade failed because of duplicated column message on task. Don't know how it addressed, but this PR could prevent the failed upgrade.

@lunny lunny added type/enhancement An improvement of existing functionality backport/v1.15 labels Dec 1, 2021
@lunny lunny mentioned this pull request Dec 1, 2021
Merged
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 1, 2021
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 1, 2021 •
edited
Loading

We should also drop the errors column if it exists.

And more comments about why we need this hacky patch.

@lunny
Copy link
Member Author

lunny commented Dec 1, 2021

We should also drop the errors column if it exists.

And more comments about why we need this hacky patch.

Since it has been renamed, if message exist, errors should have been dropped.

@wxiaoguang
Copy link
Contributor

But the users do meet the bug that message and errors co-exist. That's how the bug stops users from upgrading.

@lunny
Copy link
Member Author

lunny commented Dec 1, 2021

But the users do meet the bug that message and errors co-exist. That's how the bug stops users from upgrading.

done.

wxiaoguang
wxiaoguang approved these changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM. But I still think we had better comment this patch about why we need it. Otherwise developers in future will be confused why we wrote the strange logic today.

@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 Dec 1, 2021
@lunny
Copy link
Member Author

lunny commented Dec 1, 2021

LGTM. But I still think we had better comment this patch about why we need it. Otherwise developers in future will be confused why we wrote the strange logic today.

I will add some comments there.

@zeripath
Copy link
Contributor

zeripath commented Dec 1, 2021

It's due to them running the install page when the db already exists.

@zeripath
Copy link
Contributor

zeripath commented Dec 1, 2021

#17486 will prevent this issue.

It's just not been in a release.

@lunny lunny closed this Dec 1, 2021
@lunny lunny reopened this Dec 1, 2021
@lunny lunny force-pushed the lunny/upgrade_avoid_rename_failure branch from 26c470b to 9d5830b Compare December 2, 2021 11:57
@lunny
Copy link
Member Author

lunny commented Dec 2, 2021

make L-G-T-M work

@lunny lunny merged commit 4f98e82 into go-gitea:main Dec 2, 2021
@lunny lunny deleted the lunny/upgrade_avoid_rename_failure branch December 2, 2021 13:17
lunny added a commit to lunny/gitea that referenced this pull request Dec 2, 2021
@lunny
Check if column exist before rename if exist, just return with no err…
f24ddc7 
…or (go-gitea#17870)

* Check if column exist before rename if exist, just return with no error

* Also check if errors column exist

* Add comment for migration

* Fix sqlite test
@lunny lunny mentioned this pull request Dec 2, 2021
Merged
@lunny lunny added the backport/done All backports for this PR have been created label Dec 2, 2021
6543 pushed a commit that referenced this pull request Dec 2, 2021
@lunny
Check if column exist before rename if exist, just return with no err…
56a3b50 
…or (#17870) (#17882)

* Check if column exist before rename if exist, just return with no error

* Also check if errors column exist

* Add comment for migration

* Fix sqlite test
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@lunny
Check if column exist before rename if exist, just return with no err…
8c154c6 
…or (go-gitea#17870)

* Check if column exist before rename if exist, just return with no error

* Also check if errors column exist

* Add comment for migration

* Fix sqlite test
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lunny @wxiaoguang @zeripath @techknowlogick @GiteaBot