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

Consume returned values implicitly, remove some nolint comments #410

Merged
merged 7 commits into from
Nov 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions e2e/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,15 @@ func (c *adminClient) getJSON(ctx context.Context, path string, v interface{}) e
}

func httpGet(ctx context.Context, url string) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, err
}
resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, err
}
defer resp.Body.Close() //nolint:errcheck
defer resp.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

ack, this neither trips golint nor goland!

if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode)
}
Expand Down
2 changes: 1 addition & 1 deletion e2e/func-e_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func runTestAndInterruptEnvoy(ctx context.Context, t *testing.T, c *funcE, test

func requireEnvoyReady(ctx context.Context, t *testing.T, c *funcE) *adminClient {
adminAddressPath := filepath.Join(c.runDir, "admin-address.txt")
adminAddress, err := os.ReadFile(adminAddressPath) //nolint:gosec
adminAddress, err := os.ReadFile(adminAddressPath)
require.NoError(t, err, "error reading admin address file %q after running [%v]", adminAddressPath, c)

log.Printf("waiting for Envoy adminClient to connect after running [%v]", c)
Expand Down
2 changes: 1 addition & 1 deletion e2e/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func mockEnvoyVersionsServer() (*httptest.Server, error) {
return nil, err
}

defer f.Close() // nolint
defer f.Close()
b, err := io.ReadAll(f)
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func setHomeDir(o *globals.GlobalOpts, homeDir string) error {
}

func printVersion(c *cli.Context) {
moreos.Fprintf(c.App.Writer, "%v version %v\n", c.App.Name, c.App.Version) //nolint
moreos.Fprintf(c.App.Writer, "%v version %v\n", c.App.Name, c.App.Version)
}

var defaultFlagStringer = cli.FlagStringer
Expand Down
8 changes: 4 additions & 4 deletions internal/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ state. On exit, these archive into ` + fmt.Sprintf("`%s.tar.gz`", runDirectoryEx

for _, enableShutdownHook := range shutdown.EnableHooks {
if err := enableShutdownHook(r); err != nil {
moreos.Fprintf(r.Out, "failed to enable shutdown hook: %s\n", err) //nolint
moreos.Fprintf(r.Out, "failed to enable shutdown hook: %s\n", err)
}
}

Expand Down Expand Up @@ -136,7 +136,7 @@ func setEnvoyVersion(ctx context.Context, o *globals.GlobalOpts) (err error) {
}

// First time install: look up the latest version, which may be newer than version.LastKnownEnvoy!
o.Logf("looking up the latest Envoy version\n") //nolint
o.Logf("looking up the latest Envoy version\n")
var evs *version.ReleaseVersions
if evs, err = o.GetEnvoyVersions(ctx); err != nil {
return fmt.Errorf("couldn't lookup the latest Envoy version from %s: %w", o.EnvoyVersionsURL, err)
Expand All @@ -154,7 +154,7 @@ func setEnvoyVersion(ctx context.Context, o *globals.GlobalOpts) (err error) {
// NOTE: Warnings and errors include the platform because a release isn't available at the same time for all platforms.
func ensurePatchVersion(ctx context.Context, o *globals.GlobalOpts, v version.Version) (version.PatchVersion, error) {
if mv, ok := v.(version.MinorVersion); ok {
o.Logf("looking up the latest patch for Envoy version %s\n", mv) //nolint
o.Logf("looking up the latest patch for Envoy version %s\n", mv)
evs, err := o.GetEnvoyVersions(ctx)
var patchVersions []version.PatchVersion
if err == nil {
Expand All @@ -171,7 +171,7 @@ func ensurePatchVersion(ctx context.Context, o *globals.GlobalOpts, v version.Ve
patchVersions = append(patchVersions, r.version)
}
if pv := version.FindLatestPatchVersion(patchVersions, mv); pv != "" {
o.Logf("couldn't look up an Envoy release for version %s on platform %s: using last installed version\n", mv, o.Platform) //nolint
o.Logf("couldn't look up an Envoy release for version %s on platform %s: using last installed version\n", mv, o.Platform)
return pv, nil
}
}
Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ func NewVersionsCmd(o *globals.GlobalOpts) *cli.Command {
// TODO: handle when currentVersion is a MinorVersion
pv, ok := currentVersion.(version.PatchVersion)
if ok && vr.version == pv {
moreos.Fprintf(w, "* %s %s (set by %s)\n", vr.version, vr.releaseDate, currentVersionSource) //nolint
moreos.Fprintf(w, "* %s %s (set by %s)\n", vr.version, vr.releaseDate, currentVersionSource)
} else {
moreos.Fprintf(w, " %s %s\n", vr.version, vr.releaseDate) //nolint
moreos.Fprintf(w, " %s %s\n", vr.version, vr.releaseDate)
}
}
return w.Flush()
Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/which.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ func NewWhichCmd(o *globals.GlobalOpts) *cli.Command {
if err != nil {
return err
}
_, err = moreos.Fprintf(o.Out, "%s\n", ev)
return err
moreos.Fprintf(o.Out, "%s\n", ev)
return nil
},
CustomHelpTemplate: moreos.Sprintf(cli.CommandHelpTemplate),
}
Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
// httpGet adds the userAgent header to the request, so that we can tell what is a dev build vs release.
func httpGet(ctx context.Context, url string, p version.Platform, v string) (*http.Response, error) {
// #nosec -> url can be anywhere by design
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions internal/envoy/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ func InstallIfNeeded(ctx context.Context, o *globals.GlobalOpts) (string, error)
if err = os.MkdirAll(installPath, 0o750); err != nil {
return "", fmt.Errorf("unable to create directory %q: %w", installPath, err)
}
o.Logf("downloading %s\n", tarballURL) //nolint
o.Logf("downloading %s\n", tarballURL)
if err = untarEnvoy(ctx, installPath, tarballURL, sha256Sum, o.Platform, o.Version); err != nil { //nolint
return "", err
}
if err = os.Chtimes(installPath, mtime, mtime); err != nil { // overwrite the mtime to preserve it in the list
return "", fmt.Errorf("unable to set date of directory %q: %w", installPath, err)
}
case err == nil:
o.Logf("%s is already downloaded\n", v) //nolint
o.Logf("%s is already downloaded\n", v)
default:
// TODO: figure out how to get a stat error that isn't file not exist so we can test this
return "", err
Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func awaitAdminAddress(sigCtx context.Context, r *Runtime) {
for i := 0; i < 10 && sigCtx.Err() == nil; i++ {
adminAddress, adminErr := r.GetAdminAddress()
if adminErr == nil {
moreos.Fprintf(r.Out, "discovered admin address: %s\n", adminAddress) //nolint
Copy link
Contributor

Choose a reason for hiding this comment

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

odd this passes lint (via make check).. maybe it won't after rebase 😎

moreos.Fprintf(r.Out, "discovered admin address: %s\n", adminAddress)
return
}
time.Sleep(200 * time.Millisecond)
Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func (r *Runtime) GetAdminAddress() (string, error) {
if r.adminAddress != "" { // We don't expect the admin address to change once written, so cache it.
return r.adminAddress, nil
}
adminAddress, err := os.ReadFile(r.adminAddressPath) //nolint:gosec
adminAddress, err := os.ReadFile(r.adminAddressPath)
if err != nil {
return "", fmt.Errorf("unable to read %s: %w", r.adminAddressPath, err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func TestString(t *testing.T) {
cmdRunning.cmd = exec.Command("cat" + moreos.Exe)
cmdRunning.cmd.SysProcAttr = moreos.ProcessGroupAttr()
require.NoError(t, cmdRunning.cmd.Start())
defer cmdRunning.cmd.Process.Kill() //nolint
defer cmdRunning.cmd.Process.Kill()

tests := []struct {
name string
Expand Down
6 changes: 3 additions & 3 deletions internal/envoy/shutdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (r *Runtime) handleShutdown(ctx context.Context) {
timeout, cancel := context.WithDeadline(ctx, deadline)
defer cancel()

moreos.Fprintf(r.Out, "invoking shutdown hooks with deadline %s\n", deadline.Format(dateFormat)) //nolint
moreos.Fprintf(r.Out, "invoking shutdown hooks with deadline %s\n", deadline.Format(dateFormat))

// Run each hook in parallel, logging each error
var wg sync.WaitGroup
Expand All @@ -49,7 +49,7 @@ func (r *Runtime) handleShutdown(ctx context.Context) {
go func() {
defer wg.Done()
if err := f(timeout); err != nil {
moreos.Fprintf(r.Out, "failed shutdown hook: %s\n", err) //nolint
moreos.Fprintf(r.Out, "failed shutdown hook: %s\n", err)
}
}()
}
Expand All @@ -58,7 +58,7 @@ func (r *Runtime) handleShutdown(ctx context.Context) {

func (r *Runtime) interruptEnvoy() {
p := r.cmd.Process
moreos.Fprintf(r.Out, "sending interrupt to envoy (pid=%d)\n", p.Pid) //nolint
moreos.Fprintf(r.Out, "sending interrupt to envoy (pid=%d)\n", p.Pid)
r.maybeWarn(moreos.Interrupt(p))
}

Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/shutdown/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func copyURLToFile(ctx context.Context, url, fullPath string) error {
defer f.Close() //nolint

// #nosec -> adminAddress is written by Envoy and the paths are hard-coded
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return fmt.Errorf("could not create request %v: %w", url, err)
}
Expand Down
10 changes: 3 additions & 7 deletions internal/envoy/shutdown/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,19 +160,15 @@ func parseProc(ctx context.Context, p *process.Process) (*proc, error) {
func printProcessTable(out io.Writer, parsed []*proc) error {
// Now, start writing the process table
w := tabwriter.NewWriter(out, 0, 8, 5, ' ', 0)
if _, err := moreos.Fprintf(w, "PID\tUSERNAME\tSTATUS\tRSS\tVSZ\tMINFLT\tMAJFLT\tPCPU\tPMEM\tARGS\n"); err != nil {
return err
}
moreos.Fprintf(w, "PID\tUSERNAME\tSTATUS\tRSS\tVSZ\tMINFLT\tMAJFLT\tPCPU\tPMEM\tARGS\n")

for _, p := range parsed {
status := ""
if len(p.status) > 0 {
status = p.status[0]
}
if _, err := moreos.Fprintf(w, "%v\t%v\t%v\t%v\t%v\t%v\t%v\t%.2f\t%.2f\t%v\n",
p.pid, p.username, status, p.rss, p.vms, p.minflt, p.majflt, p.pCPU, p.pMem, p.cmd); err != nil {
return err
}
moreos.Fprintf(w, "%v\t%v\t%v\t%v\t%v\t%v\t%v\t%.2f\t%.2f\t%v\n",
p.pid, p.username, status, p.rss, p.vms, p.minflt, p.majflt, p.pCPU, p.pMem, p.cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

cool this is easier to read and chance of error writing stdout isn't high

}
return w.Flush()
}
Expand Down
2 changes: 1 addition & 1 deletion internal/globals/globals.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (o *GlobalOpts) Logf(format string, a ...interface{}) {
if o.Quiet { // TODO: we may want to do scoped logging via a Context property, if this becomes common.
return
}
moreos.Fprintf(o.Out, format, a...) //nolint
moreos.Fprintf(o.Out, format, a...)
}

const (
Expand Down
8 changes: 5 additions & 3 deletions internal/moreos/moreos.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,13 @@ func Sprintf(format string, a ...interface{}) string {
}

// Fprintf is like fmt.Fprintf, but handles EOL according runtime.GOOS. See Sprintf for notes.
func Fprintf(w io.Writer, format string, a ...interface{}) (n int, err error) {
func Fprintf(w io.Writer, format string, a ...interface{}) {
if runtime.GOOS != OSWindows {
return fmt.Fprintf(w, format, a...)
_, _ = fmt.Fprintf(w, format, a...)
Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough to do this here vs //nolint as it is contained and highly reused.

return
}
return fmt.Fprint(w, Sprintf(format, a...))

_, _ = fmt.Fprint(w, Sprintf(format, a...))
}

// ProcessGroupAttr sets attributes that ensure exec.Cmd doesn't propagate signals from func-e by default.
Expand Down
8 changes: 3 additions & 5 deletions internal/moreos/moreos_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,16 +102,14 @@ func TestSprintf(t *testing.T) {
func TestFprintf(t *testing.T) {
template := "%s\n\n%s\n"
stdout := new(bytes.Buffer)
count, err := Fprintf(stdout, template, "foo", "bar")
require.NoError(t, err)
Fprintf(stdout, template, "foo", "bar")

expected := "foo\n\nbar\n"
if runtime.GOOS == OSWindows {
expected = "foo\r\n\r\nbar\r\n"
}

require.Equal(t, expected, stdout.String())
require.Equal(t, len(expected), count)
Copy link
Contributor

Choose a reason for hiding this comment

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

yep, this is no longer possible to check, but wasn't used either!

}

// TestSprintf_IdiomaticPerOS is here to ensure that the EOL translation makes sense. For example, in UNIX, we expect
Expand Down Expand Up @@ -140,7 +138,7 @@ func TestProcessGroupAttr_Interrupt(t *testing.T) {
require.NoError(t, Interrupt(cmd.Process))

// Wait for the process to die; this could error due to the interrupt signal
cmd.Wait() //nolint
_ = cmd.Wait()
require.Error(t, findProcess(cmd.Process))

// Ensure interrupting it again doesn't error
Expand All @@ -157,7 +155,7 @@ func Test_EnsureProcessDone(t *testing.T) {
require.NoError(t, EnsureProcessDone(cmd.Process))

// Wait for the process to die; this could error due to the kill signal
cmd.Wait() //nolint
_ = cmd.Wait()
require.Error(t, findProcess(cmd.Process))

// Ensure killing it again doesn't error
Expand Down
8 changes: 4 additions & 4 deletions internal/moreos/testdata/fake_func-e.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func main() {

// This is similar to main.go, except we don't import the validation error
if err := run(context.Background(), os.Args[2:]); err != nil {
moreos.Fprintf(os.Stderr, "error: %s\n", err) //nolint
moreos.Fprintf(os.Stderr, "error: %s\n", err)
os.Exit(1)
}
os.Exit(0)
Expand All @@ -54,7 +54,7 @@ func run(ctx context.Context, args []string) error {
sigCtx, stop := signal.NotifyContext(waitCtx, os.Interrupt, syscall.SIGTERM)
defer stop()

moreos.Fprintf(cmd.Stdout, "starting: %s\n", strings.Join(cmd.Args, " ")) //nolint
moreos.Fprintf(cmd.Stdout, "starting: %s\n", strings.Join(cmd.Args, " "))
if err := cmd.Start(); err != nil {
return fmt.Errorf("unable to start Envoy process: %w", err)
}
Expand Down Expand Up @@ -93,8 +93,8 @@ func run(ctx context.Context, args []string) error {
// This is a copy/paste of envoy.Runtime.interruptEnvoy() with some formatting differences.
func handleShutdown(cmd *exec.Cmd) {
p := cmd.Process
moreos.Fprintf(cmd.Stdout, "sending interrupt to envoy (pid=%d)\n", p.Pid) //nolint
moreos.Fprintf(cmd.Stdout, "sending interrupt to envoy (pid=%d)\n", p.Pid)
if err := moreos.Interrupt(p); err != nil {
moreos.Fprintf(cmd.Stdout, "warning: %s\n", err) //nolint
moreos.Fprintf(cmd.Stdout, "warning: %s\n", err)
}
}
4 changes: 2 additions & 2 deletions internal/tar/tar.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type digester struct {
func (d *digester) Read(p []byte) (n int, err error) {
n, err = d.r.Read(p)
if n > 0 { // per docs on hash.Hash, an error is impossible on Write
d.h.Write(p[:n]) //nolint
d.h.Write(p[:n])
}
return
}
Expand Down Expand Up @@ -196,7 +196,7 @@ func TarGz(dst, src string) error { //nolint dst, src order like io.Copy

// Copy the contents of the file into the tar without buffering
func cp(dst io.Writer, src fs.FS, path string, n int64) error { // dst, src order like io.Copy
f, err := src.Open(path) //nolint:gosec
f, err := src.Open(path)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/tar/tar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func TestTarGZ(t *testing.T) {

f, e := os.Open(dst)
require.NoError(t, e)
defer f.Close() //nolint
defer f.Close()

e = Untar(tempDir, f)
require.NoError(t, e)
Expand Down
4 changes: 2 additions & 2 deletions internal/test/fakebinary/testdata/fake_envoy.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func main() {
// Start a fake admin listener that write the same sort of response Envoy's /ready would, but on all endpoints.
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Envoy console messages all write to stderr. Simulate access_log_path: '/dev/stdout'
os.Stdout.Write([]byte(fmt.Sprintf("GET %s HTTP/1.1%s", r.RequestURI, lf))) //nolint
os.Stdout.Write([]byte(fmt.Sprintf("GET %s HTTP/1.1%s", r.RequestURI, lf)))

w.Header().Add("Content-Type", "text/plain; charset=UTF-8")
w.WriteHeader(200)
Expand All @@ -65,7 +65,7 @@ func main() {
}

// Echo the same line Envoy would on successful startup
os.Stderr.Write([]byte("starting main dispatch loop" + lf)) //nolint
os.Stderr.Write([]byte("starting main dispatch loop" + lf))

// Block until we receive a signal
msg := "unexpected"
Expand Down
5 changes: 1 addition & 4 deletions internal/test/morerequire/morerequire.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,7 @@ func RequireSetMtime(t *testing.T, dir, date string) {
func RequireChdir(t *testing.T, dir string) func() {
wd, err := os.Getwd()
require.NoError(t, err)

if err = os.Chdir(dir); err != nil {
require.NoError(t, err)
}
require.NoError(t, os.Chdir(dir))

return func() {
require.NoError(t, os.Chdir(wd))
Expand Down
5 changes: 3 additions & 2 deletions internal/test/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ func (s *server) funcEVersions() []byte {
}

// RequireFakeEnvoyTarGz makes a fake envoy.tar.gz
//nolint:gosec
Copy link
Contributor

Choose a reason for hiding this comment

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

this is test-only, so ok

func RequireFakeEnvoyTarGz(t *testing.T, v version.PatchVersion) ([]byte, version.SHA256Sum) {
tempDir := t.TempDir()

Expand All @@ -124,9 +125,9 @@ func RequireFakeEnvoyTarGz(t *testing.T, v version.PatchVersion) ([]byte, versio
require.NoError(t, err)

// Read the tar.gz into a byte array. This allows the mock server to set content length correctly
f, err := os.Open(tempGz) //nolint:gosec
f, err := os.Open(tempGz)
require.NoError(t, err)
defer f.Close() // nolint
defer f.Close() //nolint
b, err := io.ReadAll(f)
require.NoError(t, err)
return b, version.SHA256Sum(fmt.Sprintf("%x", sha256.Sum256(b)))
Expand Down
Loading