bugfix: corrent the import path of logrus #953

faycheng · 2018-03-23T05:01:05Z

Ⅰ. Describe what this PR did

1, rename upper-case 'Sirupsen' to 'sirupsen'
2, change import path extra/libnetwork/Godeps/_workspace/src/github.com/Sirupsen/logrus to github.com/sirupsen/logrus

Ⅱ. Does this pull request fix one issue?

fixes #949

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

pouchrobot · 2018-03-23T05:01:09Z

We found this is your first time to contribute to Pouch, @faycheng
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

CLAassistant · 2018-03-23T05:01:11Z

All committers have signed the CLA.

allencloud · 2018-03-23T05:31:04Z

Thanks a lot for your contribution. CI fails and I am afraid that :

daemon/mgr/network.go
network/mode/bridge/bridge.go
volume/core.go
please format Go code with 'gofmt -s -w'
Makefile:42: recipe for target 'fmt' failed
make: *** [fmt] Error 1
make: *** [test] Error 2

We need to gofmt the code. @faycheng

faycheng · 2018-03-23T06:16:12Z

sorry, i will solve it

codecov-io · 2018-03-23T06:16:12Z

Codecov Report

Merging #953 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #953   +/-   ##
=======================================
  Coverage   13.06%   13.06%           
=======================================
  Files         123      123           
  Lines        8235     8235           
=======================================
  Hits         1076     1076           
  Misses       7063     7063           
  Partials       96       96

Impacted Files	Coverage Δ
daemon/mgr/network.go	`3.95% <0%> (ø)`	⬆️

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 cdc2e13...8e70a80. Read the comment docs.

allencloud · 2018-03-23T06:57:26Z

Could you help to review this?
Actually It makes me feel good to simplify some code in this PR? @rudyfly 🥂

allencloud · 2018-03-23T13:07:30Z

I tested this PR on my local machine, and it works fine indeed. Please help to take a look at this. @rudyfly

But I have to say without the change brought by this PR, the master code still works fine like the follwowing:

root@i-8brpbc9t:~/go/src/github.com/alibaba/pouch# make install
GOOS=linux go build -o pouchd
go build -o pouch github.com/alibaba/pouch/cli
install
installing pouchd and pouch to /usr/local/bin
root@i-8brpbc9t:~/go/src/github.com/alibaba/pouch# git branch
* master
  new-953
root@i-8brpbc9t:~/go/src/github.com/alibaba/pouch#

allencloud · 2018-03-23T14:46:54Z

volume/core.go

 	"github.com/alibaba/pouch/pkg/client"
 	metastore "github.com/alibaba/pouch/pkg/meta"
 	"github.com/alibaba/pouch/volume/driver"
 	volerr "github.com/alibaba/pouch/volume/error"
 	"github.com/alibaba/pouch/volume/types"
 	"github.com/alibaba/pouch/volume/types/meta"
+	"github.com/sirupsen/logrus"

 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"


If you add the import pkg "github.com/sirupsen/logrus", I think you could remove this line "github.com/sirupsen/logrus", right? @faycheng

thx, i will remove it

allencloud · 2018-03-23T14:51:55Z

network/mode/bridge/bridge.go

 	"github.com/alibaba/pouch/network"
 	"github.com/alibaba/pouch/pkg/errtypes"
+	"github.com/sirupsen/logrus"


And please move this line to L15, since this is a third-party package. We have a rule of how to import packages in :

When importing packages, to improve readabilities, we should import package by sequence: system packages, project's own packages and third-party packages. And we should keep a blank line among these three kinds of packages;

In https://github.com/alibaba/pouch/blob/master/docs/contributions/code_styles.md#additional-style-rules

allencloud · 2018-03-23T15:45:31Z

daemon/mgr/network.go

@@ -23,6 +22,7 @@ import (
 	networktypes "github.com/docker/libnetwork/types"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	netlog "github.com/sirupsen/logrus"


I think L24 and L25 are duplicated as well.

Signed-off-by: 程飞 <fay.cheng.cn@gmail.com>

allencloud · 2018-03-23T23:57:21Z

LGTM

rudyfly · 2018-03-24T00:56:15Z

daemon/mgr/network.go

@@ -411,15 +410,15 @@ func (nm *NetworkManager) getNetworkSandbox(id string) libnetwork.Sandbox {

 func initNetworkLog(cfg *config.Config) {
 	if cfg.Debug {
-		netlog.SetLevel(netlog.DebugLevel)
+		logrus.SetLevel(logrus.DebugLevel)


Libnetwork's logrus version is different from pouchd, so we need to set libnetwork's logrus addintionly.

pouchrobot added areas/log kind/bug This is bug report for project size/XS labels Mar 23, 2018

faycheng force-pushed the correct-sirupsen-import branch from 1b1ef32 to d88707a Compare March 23, 2018 05:49

faycheng force-pushed the correct-sirupsen-import branch from d88707a to 8befccc Compare March 23, 2018 06:19

allencloud assigned rudyfly Mar 23, 2018

allencloud reviewed Mar 23, 2018

View reviewed changes

faycheng force-pushed the correct-sirupsen-import branch from 8befccc to 0ab4da8 Compare March 23, 2018 15:34

pouchrobot added size/S and removed size/XS labels Mar 23, 2018

allencloud reviewed Mar 23, 2018

View reviewed changes

bugfix: corrent the import path of logrus

8e70a80

Signed-off-by: 程飞 <fay.cheng.cn@gmail.com>

faycheng force-pushed the correct-sirupsen-import branch from 0ab4da8 to 8e70a80 Compare March 23, 2018 16:17

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Mar 23, 2018

allencloud merged commit f1cee76 into AliyunContainerService:master Mar 23, 2018

rudyfly reviewed Mar 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: corrent the import path of logrus #953

bugfix: corrent the import path of logrus #953

faycheng commented Mar 23, 2018

pouchrobot commented Mar 23, 2018

CLAassistant commented Mar 23, 2018 •

edited

Loading

allencloud commented Mar 23, 2018

faycheng commented Mar 23, 2018

codecov-io commented Mar 23, 2018 •

edited

Loading

allencloud commented Mar 23, 2018

allencloud commented Mar 23, 2018 •

edited

Loading

allencloud Mar 23, 2018

faycheng Mar 23, 2018

allencloud Mar 23, 2018

allencloud Mar 23, 2018

allencloud commented Mar 23, 2018

rudyfly Mar 24, 2018

faycheng Mar 24, 2018

bugfix: corrent the import path of logrus #953

bugfix: corrent the import path of logrus #953

Conversation

faycheng commented Mar 23, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

pouchrobot commented Mar 23, 2018

CLAassistant commented Mar 23, 2018 • edited Loading

allencloud commented Mar 23, 2018

faycheng commented Mar 23, 2018

codecov-io commented Mar 23, 2018 • edited Loading

Codecov Report

allencloud commented Mar 23, 2018

allencloud commented Mar 23, 2018 • edited Loading

allencloud Mar 23, 2018

Choose a reason for hiding this comment

faycheng Mar 23, 2018

Choose a reason for hiding this comment

allencloud Mar 23, 2018

Choose a reason for hiding this comment

allencloud Mar 23, 2018

Choose a reason for hiding this comment

allencloud commented Mar 23, 2018

rudyfly Mar 24, 2018

Choose a reason for hiding this comment

faycheng Mar 24, 2018

Choose a reason for hiding this comment

CLAassistant commented Mar 23, 2018 •

edited

Loading

codecov-io commented Mar 23, 2018 •

edited

Loading

allencloud commented Mar 23, 2018 •

edited

Loading