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

Move serv hook functionality & drop GitLogger #6993

Merged
merged 34 commits into from
Jun 1, 2019

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 19, 2019

This PR moves most of the functionality of serv and hook into private routers. This reduces the number of http calls performed by these actions and allows for better logging of these actions.

In doing so it is now possible to remove the GitLogger. The only place where there is a possible reduction in functionality is in the handling of failures in enable-pprof but these are likely to be rare and not sufficient reason to keep this around.

(Fixes for LFS locks on SSH were broken out as #6999 and have been merged into master.)
(Improvements to git_test were broken out as #7086 and have been merged into master.)

@codecov-io
Copy link

codecov-io commented May 19, 2019

Codecov Report

Merging #6993 into master will decrease coverage by 0.27%.
The diff coverage is 34.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6993      +/-   ##
==========================================
- Coverage   41.84%   41.56%   -0.28%     
==========================================
  Files         443      446       +3     
  Lines       60158    60770     +612     
==========================================
+ Hits        25176    25262      +86     
- Misses      31692    32223     +531     
+ Partials     3290     3285       -5
Impacted Files Coverage Δ
modules/private/internal.go 0% <ø> (ø)
routers/init.go 68.05% <ø> (-0.44%) ⬇️
modules/log/log.go 72.07% <ø> (+0.64%) ⬆️
modules/private/serv.go 0% <0%> (ø)
routers/repo/http.go 38.07% <0%> (ø) ⬆️
modules/private/hook.go 0% <0%> (ø)
modules/private/key.go 0% <0%> (ø)
models/helper_environment.go 85.71% <100%> (ø) ⬆️
routers/private/internal.go 20.75% <100%> (-35.7%) ⬇️
routers/private/serv.go 34.59% <34.59%> (ø)
... and 15 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 8a343dd...c09a5f3. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 19, 2019
@zeripath zeripath added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 19, 2019
@zeripath zeripath added this to the 1.9.0 milestone May 19, 2019
modules/private/key.go Outdated Show resolved Hide resolved
routers/private/hook.go Show resolved Hide resolved
routers/private/hook.go Show resolved Hide resolved
routers/private/hook.go Show resolved Hide resolved
routers/private/serv.go Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented May 20, 2019

The change is so big and I think we need SSH tests.

@zeripath
Copy link
Contributor Author

zeripath commented May 20, 2019

We already do have some basic SSH tests: integrations/git_test.go which do call the hooks.

I could extend these to test protected branches. I have extended and refactored TestGit to include testing of protected branches and to reduce code duplication in this test.

These changes were broken out as #7086 - and have been merged separately - thus showing that this PR does not change the semantics of serv/hook

@zeripath zeripath mentioned this pull request May 20, 2019
@zeripath
Copy link
Contributor Author

This also fixes a bug whereby on SSH you can push to archived repositories.

@zeripath
Copy link
Contributor Author

zeripath commented May 20, 2019

This PR includes #6999 and likely fixes #5478

@zeripath
Copy link
Contributor Author

zeripath commented May 20, 2019

@lunny have greatly improved the git_test - Since broken out as #7086

cmd/hook.go Outdated Show resolved Hide resolved
modules/lfs/locks.go Outdated Show resolved Hide resolved
* Ensure that the lfs files are created with a different prefix
* Reduce the replication in git_test.go
* Remove unnecessary "/"
@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 Jun 1, 2019
@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 Jun 1, 2019
@lunny lunny merged commit 356854f into go-gitea:master Jun 1, 2019
@zeripath zeripath deleted the move-serv-hook-functionality branch June 2, 2019 14:54
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
* Move hook functionality internally

* Internalise serv logic

* Remove old internal paths

* finally remove the gitlogger

* Disallow push on archived repositories

* fix lint error

* Update modules/private/key.go

* Update routers/private/hook.go

* Update routers/private/hook.go

* Update routers/private/hook.go

* Updated routers/private/serv.go

* Fix LFS Locks over SSH

* rev-list needs to be run by the hook process

* fixup

* Improve git test

* Ensure that the lfs files are created with a different prefix

* Reduce the replication in git_test.go

* slight refactor

* Remove unnecessary "/"

* Restore ensureAnonymousClone

* Restore ensureAnonymousClone

* Run rev-list on server side

* Try passing in the alternative directories instead

* Mark test as skipped

* Improve git test

* Ensure that the lfs files are created with a different prefix
* Reduce the replication in git_test.go
* Remove unnecessary "/"
@gabyx
Copy link

gabyx commented Sep 26, 2019

It would be nice if, any reviewer who reviewed this ticket, could draw the attention to #8273.

@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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When pushing only the first branch/tag gets PR/Release marked LFS pull fails on public SSH-cloned repo
6 participants