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

Added option to disable migrations #13114

Merged
merged 9 commits into from
Dec 21, 2020
Merged

Added option to disable migrations #13114

merged 9 commits into from
Dec 21, 2020

Conversation

pboguslawski
Copy link
Contributor

@pboguslawski pboguslawski commented Oct 12, 2020 •
edited
Loading

This patch introduces DISABLE_MIGRATIONS parameter in [repository]
section of app.ini (by default set to false). If set to true
it blocks access to repository migration feature.

This mod hides also local repo import option in user editor if
local repo importing or migrations is disabled.

Author-Change-Id: IB#1105130

@pboguslawski
Added option to disable migrations
04b04cf 
This patch introduces DISABLE_MIGRATIONS parameter in [repository]
section of app.ini (by default set to false). If set to true
it blocks access to repository migration feature.

This mod hides also local repo import option in user editor if
local repo importing or migrations is disabled.

Author-Change-Id: IB#1105130
@6543
Copy link
Member

6543 commented Oct 12, 2020

make fmt needed

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2020
@6543 6543 added the type/enhancement An improvement of existing functionality label Oct 12, 2020
@pboguslawski
Code formatting fixed
0383b06 
make fmt applied

Fixes: 04b04cf
Author-Change-Id: IB#1105130
@pboguslawski
Copy link
Contributor Author

make fmt done

@6543
Copy link
Member

6543 commented Oct 13, 2020

I think you have missed to add an import to some file

@pboguslawski
Missing import added
8e62ac8 
Missing import added.

Fixes: 04b04cf
Author-Change-Id: IB#1105130
@pboguslawski
Copy link
Contributor Author

Missing import added.

@6543 6543 modified the milestones: 1.14, 1.14.0 Oct 13, 2020
@codecov-io
Copy link

codecov-io commented Oct 13, 2020 •
edited
Loading

Codecov Report

Merging #13114 into master will decrease coverage by 0.71%.
The diff coverage is 29.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13114      +/-   ##
==========================================
- Coverage   42.84%   42.12%   -0.72%     
==========================================
  Files         662      689      +27     
  Lines       73002    75830    +2828     
==========================================
+ Hits        31279    31946     +667     
- Misses      36652    38648    +1996     
- Partials     5071     5236     +165
Impacted Files Coverage Δ
modules/setting/repository.go 56.36% <ø> (ø)
routers/api/v1/repo/migrate.go 42.02% <0.00%> (-0.94%) ⬇️
routers/repo/migrate.go 39.56% <11.11%> (-1.84%) ⬇️
routers/admin/users.go 25.94% <50.00%> (+0.22%) ⬆️
modules/context/context.go 57.29% <100.00%> (+0.23%) ⬆️
modules/cron/tasks_basic.go 87.50% <100.00%> (+0.14%) ⬆️
modules/git/tree_blob.go 72.50% <0.00%> (-15.74%) ⬇️
modules/queue/helper.go 47.36% <0.00%> (-14.71%) ⬇️
routers/init.go 52.06% <0.00%> (-14.61%) ⬇️
modules/queue/setting.go 31.25% <0.00%> (-13.75%) ⬇️
... and 169 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 95ff559...0024753. Read the comment docs.

6543
6543 reviewed Oct 23, 2020
View reviewed changes
custom/conf/app.example.ini Outdated Show resolved Hide resolved
a1012112796
a1012112796 reviewed Oct 24, 2020
View reviewed changes
routers/repo/migrate.go Outdated
@@ -25,6 +26,11 @@ const (

// Migrate render migration of repository page
func Migrate(ctx *context.Context) {
if setting.Repository.DisableMigrations {
ctx.ServerError("MigratePost", fmt.Errorf("cannot migrate; migrations disabled"))
Copy link
Member

Choose a reason for hiding this comment

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

maybe it's not server error, just return 403 is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 500 to 403.

@pboguslawski
Example config fixed
62f1bcb 
DISABLE_MIGRATIONS set to false in example config to
match its default value.

Fixes: 04b04cf
Related: go-gitea#13114 (review)
Author-Change-Id: IB#1105130
@pboguslawski
HTTP error 403 instead of 500 on denied access to migration
0024753 
HTTP error 403 instead of 500 on denied access to migration.

Fixes: 04b04cf
Related: go-gitea#13114 (review)
Author-Change-Id: IB#1105130
@6543
Copy link
Member

6543 commented Oct 26, 2020 •
edited
Loading

@pboguslawski some global settings are exposed via settings APIs, i think this should be exposed too :)

#13114 (comment)

After this I think I'm fine with it

@pboguslawski
Parameter DISABLE_MIGRATIONS exposed via API
07760e7 
Parameter DISABLE_MIGRATIONS is now exposed via API.

Fixes: 04b04cf
Related: go-gitea#13114 (comment)
Author-Change-Id: IB#1105130
@pboguslawski
Copy link
Contributor Author

some global settings are exposed via settings APIs, i think this should be exposed too :)

Fixed.

@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 Oct 30, 2020
@pboguslawski
Parameter DISABLE_MIGRATIONS exposed via API fixed
b73b0e4 
Done make generate-swagger.

Fixes: 07760e7
Related: go-gitea#13114 (comment)
Author-Change-Id: IB#1105130
@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 20, 2020
@lunny
Copy link
Member

lunny commented Dec 20, 2020

Please update the branch

@pboguslawski
Copy link
Contributor Author

Just merged current master to this PR.

@6543
Copy link
Member

6543 commented Dec 21, 2020

@pboguslawski can you do this again or give maintainers write permissions to this pull?

@pboguslawski
Copy link
Contributor Author

Didn't find a way to allow rw access for maintainers to thist PR. Just pressed "Update branch" in gihtub - please check if its ok now.

@a1012112796
Copy link
Member

Didn't find a way to allow rw access for maintainers to thist PR. Just pressed "Update branch" in gihtub - please check if its ok now.

Isn't it?
image

@pboguslawski
Copy link
Contributor Author

No such option in my PR view:

image

@6543
Copy link
Member

6543 commented Dec 21, 2020

@pboguslawski I think this is because the pull is from a fork owned not by you but an org - a github issue ...

@6543 6543 merged commit 839daa8 into go-gitea:master Dec 21, 2020
@pboguslawski pboguslawski deleted the master-IB#1105130 branch December 21, 2020 15:12
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@a1012112796 a1012112796 a1012112796 left review comments

@lunny lunny lunny approved these changes

@6543 6543 6543 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/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pboguslawski @6543 @codecov-io @lunny @a1012112796 @GiteaBot