-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add boltdb hook #573
Add boltdb hook #573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logs look fine. I will review again when logs of VPN apps are changed.
pkg/app/appcommon/log_store.go
Outdated
timeLayout = time.RFC3339Nano | ||
) | ||
|
||
var re = regexp.MustCompile(`\x1b?\[[0-9;]*m`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please use any library instead? For example, this one: https://github.com/acarl005/stripansi. The regular expression there could cover more cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkryuchkov I think it better just copy their regular expression.
pkg/app/appcommon/log_store.go
Outdated
@@ -24,7 +29,8 @@ func NewProcLogger(conf ProcConfig) (*logging.MasterLogger, LogStore) { | |||
|
|||
log := logging.NewMasterLogger() | |||
log.Logger.Formatter.(*logging.TextFormatter).TimestampFormat = time.RFC3339Nano | |||
log.SetOutput(io.MultiWriter(os.Stdout, db)) | |||
// log.SetOutput(io.MultiWriter(os.Stdout, db)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this line is not needed, could you please remove it?
pkg/app/appcommon/log_store.go
Outdated
@@ -24,7 +29,8 @@ func NewProcLogger(conf ProcConfig) (*logging.MasterLogger, LogStore) { | |||
|
|||
log := logging.NewMasterLogger() | |||
log.Logger.Formatter.(*logging.TextFormatter).TimestampFormat = time.RFC3339Nano |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a part of your PR, but I think we could change time.RFC3339Nano
to timeLayout
here.
cmd/apps/vpn-client/vpn-client.go
Outdated
} | ||
|
||
serverPK := cipher.PubKey{} | ||
if err := serverPK.UnmarshalText([]byte(*serverPKStr)); err != nil { | ||
log.WithError(err).Fatalln("Invalid VPN server pub key") | ||
fmt.Printf("Invalid VPN server pub key. error = %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please change the message to Invalid VPN server pub key: %v
and apply the same format to other log messages? The format is fmt.Printf("Error log message: %v", err)
.
internal/vpn/client.go
Outdated
@@ -214,14 +214,14 @@ func (c *Client) Serve() error { | |||
defer close(connToTunDoneCh) | |||
|
|||
if _, err := io.Copy(tun, c.conn); err != nil { | |||
c.log.WithError(err).Errorf("Error resending traffic from TUN %s to VPN server", tun.Name()) | |||
fmt.Printf("Error resending traffic from TUN %s to VPN server. error = %v", tun.Name(), err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be formatted. Could you please run goimports
for this file? There's also an extra bracket at the end of the line.
internal/vpn/client.go
Outdated
} | ||
}() | ||
go func() { | ||
defer close(tunToConnCh) | ||
|
||
if _, err := io.Copy(c.conn, tun); err != nil { | ||
c.log.WithError(err).Errorf("Error resending traffic from VPN server to TUN %s", tun.Name()) | ||
fmt.Printf("Error resending traffic from VPN server to TUN %s. error = %v", tun.Name(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be formatted too.
internal/vpn/client.go
Outdated
@@ -255,14 +255,14 @@ func (c *Client) routeTrafficThroughTUN(tunGateway net.IP) error { | |||
} | |||
|
|||
func (c *Client) routeTrafficDirectly(tunGateway net.IP) { | |||
c.log.Infoln("Routing all traffic through default network gateway") | |||
fmt.Println("Routing all traffic through default network gateway") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be formatted.
cmd/apps/skysocks/skysocks.go
Outdated
@@ -56,11 +57,17 @@ func main() { | |||
<-termCh | |||
|
|||
if err := srv.Close(); err != nil { | |||
log.Fatalf("Failed to close server: %v", err) | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please remove the extra error check?
cmd/apps/skysocks/skysocks.go
Outdated
} | ||
}() | ||
|
||
if err := srv.Serve(l); err != nil { | ||
log.Fatal(err) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra error check.
cmd/apps/vpn-client/vpn-client.go
Outdated
} | ||
|
||
localPK := cipher.PubKey{} | ||
if *localPKStr != "" { | ||
if err := localPK.UnmarshalText([]byte(*localPKStr)); err != nil { | ||
log.WithError(err).Fatalln("Invalid local PK") | ||
fmt.Printf("Invalid local PK. error = %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please change the output format here and for the rest of the file?
Did you run
make format && make check
?yes
Fixes inconsistent logging in terminal
Changes:
How to test this PR:
Just run skywire-visor