chore: appease the linter, pt 1 of ? #922
Conversation
| nm = nil | ||
| } | ||
|
|
||
| log.Close() |
There was a problem hiding this comment.
Dont we have to defer log.close() this ?
There was a problem hiding this comment.
since the above for loop has no break this is unreachable code and we don't currently ever call Close().
so i don't know if we need to, but if it hasn't been a problem i would guess no.
There was a problem hiding this comment.
its good practice to have defer log.Close()..if someone changes this behaviour tomorrow, then they may fail to add log.close()
There was a problem hiding this comment.
@tamilmani1989 I read through the log package and I don't think it's safe to call log.Close() (even deferred) the way that it's written right now. We could accidentally close stdout or stderr and prevent the go runtime from writing the crash logs during a panic. That needs some work and is out of scope for this linting fix imo.
| nm = nil | ||
| } | ||
|
|
||
| log.Close() |
There was a problem hiding this comment.
its good practice to have defer log.Close()..if someone changes this behaviour tomorrow, then they may fail to add log.close()
| // Relay these incoming signals to OS signal channel. | ||
| osSignalChannel := make(chan os.Signal, 1) | ||
| signal.Notify(osSignalChannel, os.Interrupt, os.Kill, syscall.SIGTERM) | ||
| signal.Notify(osSignalChannel, os.Interrupt, syscall.SIGTERM) |
There was a problem hiding this comment.
os.Kill cannot be trapped, so including it in the Notify list is ineffective.
| // Enable ip forwading on linux vm. | ||
| // sysctl -w net.ipv4.ip_forward=1 | ||
| cmd := fmt.Sprintf(enableIPForwardCmd) | ||
| cmd := fmt.Sprint(enableIPForwardCmd) |
There was a problem hiding this comment.
why do we change this? what's the use?
There was a problem hiding this comment.
we are not doing a string interpolation so it's unnecessary to call Sprintf, and using the minimal Sprint is more correct
tamilmani1989
left a comment
tamilmani1989
left a comment
There was a problem hiding this comment.
cni changes lgtm... leaving to vamsi/mat to sign off npm changes
vakalapa
left a comment
vakalapa
left a comment
There was a problem hiding this comment.
lgtm, please wait for all the tests to complete and then you can merge.
Reason for Change:
A batch of cosmetic/non-functional changes to fix existing syntactic/style issues discovered by the linter.
Issue Fixed:
Requirements:
Notes: