Moving log and fmt PrintX to be logrus Info and Debug #796

paganotoni · 2017-12-15T02:59:48Z

This is related with #765

markbates

Overall it looks good. Note that the Siruspen repo was moved a while ago to siruspen so all of the import statements in this PR are a regression, so that needs to be fixed. There are also a bunch of places where errors are being logged, those should go to the Error logger. I also found at least one place where the output shouldn't go to the logger, but is meant to be printed to the screen as is.

markbates · 2017-12-15T15:48:16Z

app.go


 		err := a.Stop(ctx.Err())
 		if err != nil {
-			fmt.Println(err)
+			logrus.Debug(err)


This should probably be sent to the Error logger.

markbates · 2017-12-15T15:48:27Z

app.go

 			err = a.Worker.Stop()
 			if err != nil {
-				fmt.Println(err)
+				logrus.Debug(err)


This should probably be sent to the Error logger.

markbates · 2017-12-15T15:48:31Z

app.go

 			}
 		}

 		err = server.Shutdown(ctx)
 		if err != nil {
-			fmt.Println(err)
+			logrus.Debug(err)


This should probably be sent to the Error logger.

markbates · 2017-12-15T15:48:37Z

app.go

@@ -125,7 +126,7 @@ func (a *App) Serve() error {
 func (a *App) Stop(err error) error {
 	a.cancel()
 	if err != nil && errors.Cause(err) != context.Canceled {
-		fmt.Println(err)
+		logrus.Debug(err)


This should probably be sent to the Error logger.

markbates · 2017-12-15T15:49:28Z

buffalo/cmd/build/target.go

@@ -4,7 +4,7 @@ import (
 	"os"
 	"path/filepath"

-	"github.com/sirupsen/logrus"
+	"github.com/Sirupsen/logrus"


The repo is now lower case sirupsen. Can you please undo these import changes?

markbates · 2017-12-15T15:52:48Z

plugins/plugins.go

@@ -83,12 +83,12 @@ func askBin(path string) Commands {
 	cmd.Stderr = bb
 	err := cmd.Run()
 	if err != nil {
-		fmt.Printf("[PLUGIN] error loading plugin %s: %s\n%s\n", path, err, bb.String())
+		logrus.Infof("[PLUGIN] error loading plugin %s: %s\n%s\n", path, err, bb.String())


Error logger

markbates · 2017-12-15T15:52:57Z

plugins/plugins.go

 		return commands
 	}
 	err = json.NewDecoder(bb).Decode(&commands)
 	if err != nil {
-		fmt.Printf("[PLUGIN] error loading plugin %s: %s\n", path, err)
+		logrus.Infof("[PLUGIN] error loading plugin %s: %s\n", path, err)


error logger

markbates · 2017-12-15T15:53:03Z

render/template.go

 	"os"
 	"path/filepath"
 	"sort"
 	"strings"

+	"github.com/Sirupsen/logrus"


The repo is now lower case sirupsen. Can you please undo these import changes?

markbates · 2017-12-15T15:53:10Z

request_logger.go

@@ -3,10 +3,10 @@ package buffalo
 import (
 	"time"

+	"github.com/Sirupsen/logrus"


The repo is now lower case sirupsen. Can you please undo these import changes?

markbates · 2017-12-15T15:53:15Z

worker/simple.go

@@ -5,8 +5,8 @@ import (
 	"sync"
 	"time"

+	"github.com/Sirupsen/logrus"


The repo is now lower case sirupsen. Can you please undo these import changes?

paganotoni · 2017-12-16T17:30:26Z

@markbates thanks for the feedback, all these have been implemented, let me know how is it looking when you have the chance, thanks again!

markbates · 2017-12-18T15:51:31Z

buffalo/cmd/build.go

@@ -57,7 +56,7 @@ var xbuildCmd = &cobra.Command{
 			return errors.WithStack(err)
 		}

-		fmt.Printf("\nYou application was successfully built at %s\n", filepath.Join(b.Root, b.Bin))
+		logrus.Info("\nYou application was successfully built at %s\n", filepath.Join(b.Root, b.Bin))


this needs to be Infof

markbates · 2017-12-18T15:51:54Z

buffalo/cmd/build/apkg.go

@@ -91,7 +91,7 @@ func (b *Builder) buildADatabase() error {
 			logrus.Debugf("no sqlite usage in database.yml detected, applying the nosqlite tag")
 			b.Tags = append(b.Tags, "nosqlite")
 		} else if !b.Static {
-			fmt.Println("you are building a SQLite application, please consider using the `--static` flag to compile a static binary")
+			logrus.Debugf("you are building a SQLite application, please consider using the `--static` flag to compile a static binary")


This can be just Debug

markbates · 2017-12-18T15:52:08Z

buffalo/cmd/build/apkg.go

@@ -91,7 +91,7 @@ func (b *Builder) buildADatabase() error {
 			logrus.Debugf("no sqlite usage in database.yml detected, applying the nosqlite tag")
 			b.Tags = append(b.Tags, "nosqlite")
 		} else if !b.Static {
-			fmt.Println("you are building a SQLite application, please consider using the `--static` flag to compile a static binary")
+			logrus.Debugf("you are building a SQLite application, please consider using the `--static` flag to compile a static binary")
 		}
 	} else {
 		logrus.Debugf("no database.yml detected, applying the nosqlite tag")


This can be just Debug

markbates · 2017-12-18T15:52:27Z

buffalo/cmd/destroy/resource.go

@@ -51,22 +52,22 @@ func confirm(msg string) bool {
 func removeTemplates(fileName string) {
 	if YesToAll || confirm("Want to remove templates? (Y/n)") {
 		templatesFolder := fmt.Sprintf(filepath.Join("templates", fileName))
-		fmt.Printf("- Deleted %v folder\n", templatesFolder)
+		logrus.Info("- Deleted %v folder\n", templatesFolder)


This needs to be Infof

markbates · 2017-12-18T15:52:51Z

buffalo/cmd/destroy/resource.go

 		os.Remove(filepath.Join("actions", fmt.Sprintf("%v_test.go", fileName)))

 		content, err := ioutil.ReadFile(filepath.Join("actions", "app.go"))
 		if err != nil {
-			fmt.Println("[WARNING] error reading app.go content")
+			logrus.Info("[WARNING] error reading app.go content")


This should be Warn and we can remove the warning label

markbates · 2017-12-18T15:55:16Z

generators/action/action.go

@@ -121,7 +122,7 @@ func (act Generator) findHandlersToAdd(path string) []meta.Name {
 	for _, action := range act.Actions {
 		funcSignature := fmt.Sprintf("app.GET(\"/%s/%s\", %s%s)", act.Name.URL(), action.URL(), act.Name.Camel(), action.Camel())
 		if strings.Contains(string(fileContents), funcSignature) {
-			fmt.Printf("--> [warning] skipping %s from app.go since it already exists\n", funcSignature)
+			logrus.Infof("--> [warning] skipping %s from app.go since it already exists\n", funcSignature)


This should be Warnf and we can remove the warning label

markbates · 2017-12-18T15:55:24Z

generators/action/action.go

@@ -142,7 +143,7 @@ func (act Generator) findTestsToAdd(path string) []meta.Name {
 	for _, action := range act.Actions {
 		funcSignature := fmt.Sprintf("func (as *ActionSuite) Test_%v_%v() {", act.Name.Camel(), action.Camel())
 		if strings.Contains(string(fileContents), funcSignature) {
-			fmt.Printf("--> [warning] skipping Test_%v_%v since it already exists\n", act.Name.Camel(), action.Camel())
+			logrus.Infof("--> [warning] skipping Test_%v_%v since it already exists\n", act.Name.Camel(), action.Camel())


This should be Warnf and we can remove the warning label

markbates · 2017-12-18T15:55:37Z

generators/assets/webpack/webpack.go

@@ -26,7 +26,7 @@ func (w Generator) Run(root string, data makr.Data) error {

 	// if there's no npm, return!
 	if _, err := exec.LookPath("npm"); err != nil {
-		fmt.Println("Could not find npm. Skipping webpack generation.")
+		logrus.Infof("Could not find npm. Skipping webpack generation.")


This should be Info

markbates · 2017-12-18T15:56:04Z

grifts.go

@@ -35,7 +36,7 @@ func secretGrift() {
 		rx := regexp.MustCompile(`(\W+)`)
 		bb = rx.ReplaceAll(bb, []byte(""))
 		s := randx.String(6) + string(bb)
-		fmt.Println(s[:127])
+		println(s[:127])


This should still be using fmt.Println

markbates · 2017-12-18T15:57:59Z

options.go

@@ -121,7 +122,7 @@ func optionsWithDefaults(opts Options) Options {
 		secret := envy.Get("SESSION_SECRET", "")
 		// In production a SESSION_SECRET must be set!
 		if opts.Env == "production" && secret == "" {
-			log.Println("WARNING! Unless you set SESSION_SECRET env variable, your session storage is not protected!")
+			logrus.Warn("WARNING! Unless you set SESSION_SECRET env variable, your session storage is not protected!")


we can remove the warning label

…falo into task/moving-logs-to-logrus

paganotoni · 2017-12-18T19:36:33Z

@markbates next round of changes applied :)

markbates · 2017-12-20T21:44:48Z

grifts.go

-	fmt.Println(fmt.Sprintf("-> %s", a.Name))
-	fmt.Println(a.Middleware.String())
-	fmt.Println()
+	logrus.Infof("-> %s", a.Name)


this function needs to go back to using Println.

markbates

just the one comment and then we're good to go!

…falo into task/moving-logs-to-logrus

paganotoni · 2017-12-20T22:02:01Z

@markbates all set :)

markbates · 2017-12-20T22:10:37Z

grifts.go

-	fmt.Println(fmt.Sprintf("-> %s", a.Name))
-	fmt.Println(a.Middleware.String())
-	fmt.Println()
+	fmt.Printf("-> %s ", a.Name)


Sorry for this, but this statement is missing a newline. Last change, I swear! :)

paganotoni · 2017-12-20T22:25:41Z

lol, now all set

markbates · 2017-12-20T22:26:45Z

Awesome! Thanks.

paganotoni · 2017-12-20T22:27:54Z

Thank you for all the help here!

moving fmt.PrintX to be logrus

33aedd9

paganotoni requested a review from markbates December 15, 2017 02:59

markbates suggested changes Dec 15, 2017

View reviewed changes

paganotoni and others added 5 commits December 15, 2017 16:21

moving to lowercase logrus

a3900db

changing some log types based in @markbates feedback

b7495f7

moving errors to the error level with logrus

9f5ca40

loging some other errors/failures with the Error(f) level in logrus

83112a8

Merge branch 'development' into task/moving-logs-to-logrus

8102a29

markbates suggested changes Dec 18, 2017

View reviewed changes

paganotoni and others added 5 commits December 18, 2017 12:08

applying changes from @markbates

54bd9cb

Merge github.com:gobuffalo/buffalo into task/moving-logs-to-logrus

a37c60b

Merge branch 'task/moving-logs-to-logrus' of github.com:gobuffalo/buf…

75f8a32

…falo into task/moving-logs-to-logrus

Merge branch 'development' into task/moving-logs-to-logrus

7123222

making resources pass

955d197

markbates added 2 commits December 20, 2017 10:45

Merge branch 'development' into task/moving-logs-to-logrus

85904dd

Merge branch 'development' into task/moving-logs-to-logrus

695b489

markbates reviewed Dec 20, 2017

View reviewed changes

markbates suggested changes Dec 20, 2017

View reviewed changes

paganotoni added 2 commits December 20, 2017 16:51

moving printMiddleware to use fmt.Printf

272efaa

Merge branch 'task/moving-logs-to-logrus' of github.com:gobuffalo/buf…

ae69435

…falo into task/moving-logs-to-logrus

markbates suggested changes Dec 20, 2017

View reviewed changes

adding missing newline

a44841a

markbates merged commit ec4fab3 into development Dec 20, 2017

paganotoni deleted the task/moving-logs-to-logrus branch December 20, 2017 22:27

paganotoni mentioned this pull request Dec 20, 2017

Logs formatting is not constant #765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving log and fmt PrintX to be logrus Info and Debug #796

Moving log and fmt PrintX to be logrus Info and Debug #796

paganotoni commented Dec 15, 2017

markbates left a comment

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

markbates Dec 15, 2017

paganotoni commented Dec 16, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

markbates Dec 18, 2017

paganotoni commented Dec 18, 2017

markbates Dec 20, 2017

markbates left a comment

paganotoni commented Dec 20, 2017

markbates Dec 20, 2017

paganotoni commented Dec 20, 2017

markbates commented Dec 20, 2017

paganotoni commented Dec 20, 2017

Moving log and fmt PrintX to be logrus Info and Debug #796

Moving log and fmt PrintX to be logrus Info and Debug #796

Conversation

paganotoni commented Dec 15, 2017

markbates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paganotoni commented Dec 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paganotoni commented Dec 18, 2017

Choose a reason for hiding this comment

markbates left a comment

Choose a reason for hiding this comment

paganotoni commented Dec 20, 2017

Choose a reason for hiding this comment

paganotoni commented Dec 20, 2017

markbates commented Dec 20, 2017

paganotoni commented Dec 20, 2017