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

Add migration for password algorithm change #12784

Merged
merged 13 commits into from
Sep 15, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Sep 9, 2020

#12688 changed the default for the user table leading to sync2 warnings

Unfortunately changing defaults requires a complete table rewrite in general.

However, just dropping columns could be bad - so this PR leverages the
techniques used in recreate table to recreate from the inferred schema
and recreates the user table.

This is not necessarily the correct thing to do - but code sometimes speaks
louder than words.

Signed-off-by: Andrew Thornton art27@cantab.net

go-gitea#12688 changed the default for the user table leading to sync2 warnings

Unfortunately changing defaults requires a complete table rewrite in general.

However, just dropping columns could be bad - so this PR leverages the
techniques used in recreate table to recreate from the inferred schema
and recreates the user table.

This is not necessarily the correct thing to do - but code sometimes speaks
louder than words.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
models/migrations/v150.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 9, 2020
models/migrations/v150.go Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

Tested, worked for me.

@zeripath

This comment has been minimized.

@silverwind
Copy link
Member

Maybe it's worth to move some of the code to (future) shared function?

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

OK the indices are correct for the user table

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #12784 into master will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12784      +/-   ##
==========================================
- Coverage   43.16%   43.08%   -0.08%     
==========================================
  Files         654      655       +1     
  Lines       72200    72325     +125     
==========================================
  Hits        31163    31163              
- Misses      35986    36117     +131     
+ Partials     5051     5045       -6     
Impacted Files Coverage Δ
models/migrations/migrations.go 2.46% <ø> (ø)
models/migrations/v151.go 0.00% <0.00%> (ø)
modules/indexer/stats/db.go 43.47% <0.00%> (-17.40%) ⬇️
modules/indexer/stats/queue.go 64.70% <0.00%> (-11.77%) ⬇️
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/queue/workerpool.go 58.77% <0.00%> (-1.23%) ⬇️
services/mailer/mail.go 54.83% <0.00%> (-1.08%) ⬇️
modules/git/repo.go 46.19% <0.00%> (-0.51%) ⬇️
services/pull/pull.go 40.78% <0.00%> (ø)
modules/notification/mail/mail.go 34.48% <0.00%> (ø)
... and 6 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 88823f3...4d4635b. Read the comment docs.

@lafriks lafriks added this to the 1.13.0 milestone Sep 10, 2020
@6543
Copy link
Member

6543 commented Sep 10, 2020

@zeripath does recreateTable not cover this case?

@zeripath
Copy link
Contributor Author

It's an option. I thought I'd write this to show what a non-recreate table solution looks like - this one means that any columns that are excess will not be deleted. Happy to drop the SQLite3 solution to use recreateTable though.

models/migrations/v150.go Outdated Show resolved Hide resolved
@GiteaBot 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 Sep 10, 2020
@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 Sep 11, 2020
@6543
Copy link
Member

6543 commented Sep 11, 2020

please resolve conflicts

models/migrations/v150.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Sep 15, 2020

🚀

@techknowlogick techknowlogick merged commit 772b5e0 into go-gitea:master Sep 15, 2020
@zeripath zeripath deleted the migration-for-12688 branch September 30, 2020 11:12
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 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.

9 participants