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 code.gitea.io/gitea/modules/log to code.gitea.io/log as a standalone package #6970

Closed
wants to merge 15 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented May 17, 2019

This PR will move code.gitea.io/gitea/modules/log to code.gitea.io/log as a standalone package so that other projects (i.e. tea) could use the log package but not import all the gitea packages.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 17, 2019
@lunny lunny added this to the 1.9.0 milestone May 17, 2019
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

please change imports to "code.gitea.io/log"

cmd/admin.go Outdated
@@ -15,7 +15,7 @@ import (
"code.gitea.io/gitea/modules/auth/oauth2"
"code.gitea.io/gitea/modules/generate"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"gitea.com/gitea/log"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"gitea.com/gitea/log"
"code.gitea.io/log"

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 30, 2019
@techknowlogick techknowlogick changed the title Move code.gitea.io/gitea/modules/log to gitea.com/gitea/log as a standalone package Move code.gitea.io/gitea/modules/log to code.gitea.io/log as a standalone package May 30, 2019
@lunny lunny force-pushed the lunny/move_log_dependent_pkg branch 2 times, most recently from 1c7489a to 248a0e2 Compare June 1, 2019 12:35
@codecov-io
Copy link

codecov-io commented Jun 1, 2019

Codecov Report

Merging #6970 into master will decrease coverage by 0.65%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6970      +/-   ##
==========================================
- Coverage   40.98%   40.32%   -0.66%     
==========================================
  Files         462      446      -16     
  Lines       62584    61376    -1208     
==========================================
- Hits        25648    24749     -899     
+ Misses      33574    33303     -271     
+ Partials     3362     3324      -38
Impacted Files Coverage Δ
models/unit.go 67.56% <ø> (+5.4%) ⬆️
models/repo.go 48.35% <ø> (ø) ⬆️
modules/migrations/migrate.go 23.35% <ø> (ø) ⬆️
models/u2f.go 63.15% <ø> (ø) ⬆️
models/admin.go 81.63% <ø> (ø) ⬆️
routers/init.go 65.33% <ø> (ø) ⬆️
routers/private/hook.go 47.95% <ø> (ø) ⬆️
models/branches.go 50% <ø> (ø) ⬆️
models/migrations/v45.go 0% <ø> (ø) ⬆️
models/repo_branch.go 49.53% <ø> (ø) ⬆️
... and 143 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 d145955...99fac9b. Read the comment docs.

@lunny
Copy link
Member Author

lunny commented Jun 1, 2019

Ready to review.

@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
@zeripath
Copy link
Contributor

zeripath commented Jun 1, 2019

If we can get #6993 in first we can get rid of the gitlog weirdness too.

@lunny lunny force-pushed the lunny/move_log_dependent_pkg branch from 248a0e2 to 21a6e1a Compare June 1, 2019 15:17
@lunny
Copy link
Member Author

lunny commented Jun 2, 2019

@zeripath rebased

@zeripath
Copy link
Contributor

zeripath commented Jun 2, 2019

Thanks @lunny.

@lunny lunny force-pushed the lunny/move_log_dependent_pkg branch 3 times, most recently from d93303a to 64de645 Compare June 22, 2019 07:32
@lunny lunny force-pushed the lunny/move_log_dependent_pkg branch from 99fac9b to 8e9b957 Compare June 23, 2019 01:17
@lunny lunny modified the milestones: 1.9.0, 1.10.0 Jun 23, 2019
@stale
Copy link

stale bot commented Aug 22, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Aug 22, 2019
@@ -39,7 +39,7 @@ func CheckConsistencyFor(t *testing.T, beansToCheck ...interface{}) {
ptrToSliceValue := reflect.New(sliceType)
ptrToSliceValue.Elem().Set(sliceValue)

assert.NoError(t, x.Where(bean).Find(ptrToSliceValue.Interface()))
assert.NoError(t, x.Table(bean).Find(ptrToSliceValue.Interface()))
Copy link
Member

Choose a reason for hiding this comment

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

This is relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will open a new PR to instead this one. It's unrelated but should be a bug.

Copy link
Member

Choose a reason for hiding this comment

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

@lunny can you reverence the PR to tis bugfix?

Copy link
Member

Choose a reason for hiding this comment

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

for the log -> #7271

@stale stale bot removed issue/stale labels Aug 22, 2019
@tamalsaha
Copy link
Contributor

I wonder if gitea could switch to https://github.com/go-logr/logr which defines a logging interface. Then you can plug in https://github.com/uber-go/zap using https://github.com/go-logr/zapr . zap is a very high perf logging library and this logr is getting used in the Kubernetes project.

I wonder if Gitea really needs to maintain its own logging library.

@lunny
Copy link
Member Author

lunny commented Aug 25, 2019

@tamalsaha I don't think Gitea should maintain a logging library itself except no logs could satisfy it's requirements.

@tamalsaha
Copy link
Contributor

Is there any issue or doc that explains what are the required features? Or some doc that explains the limitation of other logging libraries?

@lunny
Copy link
Member Author

lunny commented Aug 25, 2019

@tamalsaha I think we have no. Only have docs about logging https://docs.gitea.io/zh-cn/logging-configuration/

@lunny lunny modified the milestones: 1.10.0, 1.11.0 Sep 15, 2019
@lunny
Copy link
Member Author

lunny commented Nov 15, 2019

I will close this since it's difficult to rebase the PR.

@lunny lunny closed this Nov 15, 2019
@lunny lunny deleted the lunny/move_log_dependent_pkg branch November 15, 2019 03:28
@lunny lunny removed this from the 1.11.0 milestone Nov 15, 2019
@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/need 1 This PR needs approval from one additional maintainer to be merged. 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.

8 participants