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

Make Xorm log configurable #174

Merged
merged 4 commits into from
Feb 20, 2017
Merged

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 15, 2016

Before xorm's log will be stored xorm.log, now it will use the same configuration as gogs.log, except the file name from gogs.log to xorm.log. So that, user could set it to be rotated to avoid disk usage.

@codecov-io
Copy link

codecov-io commented Nov 15, 2016

Current coverage is 3.04% (diff: 0.00%)

Merging #174 into master will decrease coverage by <.01%

@@            master      #174   diff @@
========================================
  Files           33        33          
  Lines         8075      8091    +16   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           246       246          
- Misses        7809      7825    +16   
  Partials        20        20          

Powered by Codecov. Last update 07a0753...669050d

validLevels := []string{"Trace", "Debug", "Info", "Warn", "Error", "Critical"}
// Log level.
levelName := Cfg.Section("log."+mode).Key("LEVEL").In(
Cfg.Section("log").Key("LEVEL").In("Trace", validLevels),
Copy link
Member

Choose a reason for hiding this comment

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

Trace is also part of the slice above

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is the default log level

@tboerger tboerger added the type/enhancement An improvement of existing functionality label Nov 15, 2016
@tboerger tboerger added this to the 1.0.0 milestone Nov 15, 2016
Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

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

I pulled this PR locally and tried to create a repository, got:

2016/11/15 08:29:30 [...routers/repo/repo.go:101 handleCreateError()] [E] CreatePost: initRepository: initRepoCommit: git push: remote: panic: runtime error: invalid memory address or nil pointer dereference        
remote: [signal 0xb code=0x1 addr=0x0 pc=0x63f930]        
remote: 
remote: goroutine 1 [running]:        
remote: panic(0xfd7e20, 0xc8200121e0)        
remote:         /usr/lib/go-1.6/src/runtime/panic.go:481 +0x3e6        
remote: code.gitea.io/gitea/modules/log.(*XORMLogBridge).ShowSQL(0x0, 0xc82031ac68, 0x1, 0x1)        
remote:         /home/strk/go/src/code.gitea.io/gitea/modules/log/xorm.go:104 +0x40        
remote: code.gitea.io/gitea/vendor/github.com/go-xorm/xorm.(*Engine).ShowSQL(0xc8203052c0, 0xc82031ac68, 0x1, 0x1)        
remote:         /home/strk/go/src/code.gitea.io/gitea/vendor/github.com/go-xorm/xorm/engine.go:51 +0x60        
remote: code.gitea.io/gitea/models.SetEngine(0x0, 0x0)        
remote:         /home/strk/go/src/code.gitea.io/gitea/models/models.go:184 +0x2f2        
remote: code.gitea.io/gitea/cmd.setup(0x1154320, 0xa)        
remote:         /home/strk/go/src/code.gitea.io/gitea/cmd/serve.go:57 +0x103        
remote: code.gitea.io/gitea/cmd.runUpdate(0xc8202cf2c0, 0x0, 0x0)        
remote:         /home/strk/go/src/code.gitea.io/gitea/cmd/update.go:37 +0xcc        
remote: code.gitea.io/gitea/vendor/github.com/urfave/cli.HandleAction(0xe57260, 0x13233b0, 0xc8202cf2c0, 0x0, 0x0)        
remote:         /home/strk/go/src/code.gitea.io/gitea/vendor/github.com/urfave/cli/app.go:471 +0x5e        
remote: code.gitea.io/gitea/vendor/github.com/urfave/cli.Command.Run(0x1128300, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1256c80, 0x2e, 0x0, ...)        
remote:         /home/strk/go/src/code.gitea.io/gitea/vendor/github.com/urfave/cli/command.go:191 +0x10b7        
remote: code.gitea.io/gitea/vendor/github.com/urfave/cli.(*App).Run(0xc8201b7860, 0xc82000a180, 0x6, 0x6, 0x0, 0x0)        
remote:         /home/strk/go/src/code.gitea.io/gitea/vendor/github.com/urfave/cli/app.go:241 +0xb65        
remote: main.main()        
remote:         /home/strk/go/src/code.gitea.io/gitea/main.go:40 +0x3a1        
remote: error: hook declined to update refs/heads/master        
To /home/strk/gitea-repositories/strk/repo1.git
 ! [remote rejected] master -> master (hook declined)
error: failed to push some refs to '/home/strk/gitea-repositories/strk/repo1.git'

Reverting application of this PR made the new repository creation work.

@lunny
Copy link
Member Author

lunny commented Nov 16, 2016

Yes, a bug, I'm fixing.

@lunny lunny modified the milestones: 1.1.0, 1.0.0 Nov 21, 2016
@andreynering andreynering changed the title make xorm log configable Make Xorm log configurable Nov 23, 2016
@tboerger
Copy link
Member

Do we really want to delay that for 1.1.0?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2016
@strk
Copy link
Member

strk commented Nov 24, 2016

Personally I think the soon we can get to 1.0.0 the better, so anything not fundamental can wait if it takes time away from that focus.

@lunny
Copy link
Member Author

lunny commented Nov 24, 2016

I will fix this. Maybe we need change a new log system

@tboerger
Copy link
Member

What about a single logrus logger?

@tboerger
Copy link
Member

And additionally separate log levels for the different types of logs, so that you can enable general debug log, but disable debug output of xorm

@tboerger
Copy link
Member

@lunny please fix the conflicts.

@tboerger tboerger added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Dec 29, 2016
@Bwko
Copy link
Member

Bwko commented Jan 11, 2017

@lunny Could you resolve the conflicts?

@lunny
Copy link
Member Author

lunny commented Jan 12, 2017

I will update this later.

@lunny lunny force-pushed the lunny/xorm_log_config branch 2 times, most recently from 6f044ec to 787481c Compare January 26, 2017 14:37
@lunny lunny removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jan 26, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Feb 12, 2017

@lunny Any update/status on this?

@lunny
Copy link
Member Author

lunny commented Feb 12, 2017

It's ready to review. I have posted some messages on gitter.

@bkcsoft
Copy link
Member

bkcsoft commented Feb 12, 2017

Well, LGTM :)

@tboerger tboerger 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 Feb 12, 2017
logModes := strings.Split(Cfg.Section("log").Key("MODE").MustString("console"), ",")
var logConfigs string
for _, mode := range logModes {
if disableConsole && mode == "console" {
Copy link
Member

Choose a reason for hiding this comment

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

Should this check take place after calling strings.TrimSpace()?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -0,0 +1,132 @@
package log
Copy link
Member

Choose a reason for hiding this comment

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

Copyright?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@lunny
Copy link
Member Author

lunny commented Feb 19, 2017

resolved conflicts.

@strk
Copy link
Member

strk commented Feb 19, 2017

starts for me, and logs lots of things on stdout. what/how to test more of it ? is documentation ready somewhere ?

@lunny
Copy link
Member Author

lunny commented Feb 20, 2017

It's controlled by the [log] in app.ini.

@strk
Copy link
Member

strk commented Feb 20, 2017

Ok, LGTM -- I'm still seeing Macaron logs on console but I guess that's a different thing

@tboerger tboerger 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 Feb 20, 2017
@lunny lunny merged commit 04fdeb9 into go-gitea:master Feb 20, 2017
@lunny lunny deleted the lunny/xorm_log_config branch April 19, 2017 05:46
ethantkoenig pushed a commit to ethantkoenig/gitea that referenced this pull request May 30, 2017
…gitea#174)

* Restores the HashTag code in Issue comments

* Has travis test with 1.7 and 1.8 of Go
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants