fix: fix checks URL panic and other issues caught by staticcheck

Fixes canonical#579

.github/workflows/lint.yml

@@ -28,3 +28,20 @@ jobs:
               golangci-lint run -c .github/.golangci.yml --fix
           and check in the changes.'
           exit 1
+
+  staticcheck:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+
+      - uses: actions/setup-go@v4
+        with:
+          go-mod-file: 'go.mod'
+
+      - name: Install staticcheck
+        run: |
+          go install honnef.co/go/tools/cmd/[email protected]
+
+      - name: Run checks
+        run: |
+          staticcheck ./...

internals/cli/cmd_exec.go

@@ -255,7 +255,7 @@ func execControlHandler(process *client.ExecProcess, terminal bool, stop <-chan
 			err = process.SendResize(width, height)
 			if err != nil {
 				logger.Debugf("Cannot set terminal size: %v", err)
-				break
+				break //lint:ignore SA4011 Keep "ineffective" break for consistency
 			}
 		case unix.SIGHUP:
 			logger.Debugf("Received 'SIGHUP' signal, forwarding and exiting")
@@ -272,7 +272,7 @@ func execControlHandler(process *client.ExecProcess, terminal bool, stop <-chan
 			err := process.SendSignal(unix.SignalName(sig.(unix.Signal)))
 			if err != nil {
 				logger.Debugf("Cannot forward signal '%s': %v", sig, err)
-				break
+				break //lint:ignore SA4011 Keep "ineffective" break for consistency
 			}
 		}
 	}

internals/cli/cmd_help.go

@@ -96,10 +96,6 @@ func addHelp(parser *flags.Parser) error {
 	return nil
 }
 
-func (cmd *cmdHelp) setParser(parser *flags.Parser) {
-	cmd.parser = parser
-}
-
 // manfixer is a hackish way to fix drawbacks in the generated manpage:
 // - no way to get it into section 8
 // - duplicated TP lines that break older groff (e.g. 14.04), lp:1814767
@@ -150,7 +146,7 @@ func (cmd cmdHelp) Execute(args []string) error {
 	}
 	if cmd.All {
 		if len(cmd.Positional.Subs) > 0 {
-			return fmt.Errorf("help accepts a command, or '--all', but not both.")
+			return fmt.Errorf("help accepts a command, or '--all', but not both")
 		}
 		printLongHelp(cmd.parser)
 		return nil
@@ -164,7 +160,7 @@ func (cmd cmdHelp) Execute(args []string) error {
 			if x := cmd.parser.Command.Active; x != nil && x.Name != "help" {
 				sug = cmdpkg.ProgramName + " help " + x.Name
 			}
-			return fmt.Errorf("unknown command %q, see '%s'.", subname, sug)
+			return fmt.Errorf("unknown command %q, see '%s'", subname, sug)
 		}
 		// this makes "pebble help foo" work the same as "pebble foo --help"
 		cmd.parser.Command.Active = subcmd

internals/cli/cmd_run.go

@@ -126,7 +126,7 @@ func runWatchdog(d *daemon.Daemon) (*time.Ticker, error) {
 		// Not running under systemd.
 		return nil, nil
 	}
-	usec, err := strconv.ParseFloat(os.Getenv("WATCHDOG_USEC"), 10)
+	usec, err := strconv.ParseFloat(os.Getenv("WATCHDOG_USEC"), 64)
 	if usec == 0 || err != nil {
 		return nil, fmt.Errorf("cannot parse WATCHDOG_USEC: %q", os.Getenv("WATCHDOG_USEC"))
 	}

internals/cli/cmd_start-checks.go

@@ -64,7 +64,7 @@ func (cmd cmdStartChecks) Execute(args []string) error {
 
 	var summary string
 	if len(results.Changed) == 0 {
-		summary = fmt.Sprintf("Checks already started.")
+		summary = "Checks already started."
 	} else {
 		summary = fmt.Sprintf("Checks started: %s", strings.Join(results.Changed, ", "))
 	}

internals/cli/cmd_stop-checks.go

@@ -64,7 +64,7 @@ func (cmd cmdStopChecks) Execute(args []string) error {
 
 	var summary string
 	if len(results.Changed) == 0 {
-		summary = fmt.Sprintf("Checks already stopped.")
+		summary = "Checks already stopped."
 	} else {
 		summary = fmt.Sprintf("Checks stopped: %s", strings.Join(results.Changed, ", "))
 	}

internals/cli/cmd_warnings.go

@@ -87,6 +87,9 @@ func (cmd *cmdWarnings) Execute(args []string) error {
 	}
 
 	warnings, err := cmd.client.Notices(options)
+	if err != nil {
+		return fmt.Errorf("cannot get notices: %w", err)
+	}
 	if len(warnings) == 0 {
 		if cmd.All || state.WarningsLastOkayed.IsZero() {
 			fmt.Fprintln(Stderr, "No warnings.")

internals/cli/format.go

@@ -12,6 +12,8 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+//lint:file-ignore U1000 Some things here are not used, but we want to keep in sync with snapd
+
 package cli
 
 import (
@@ -188,7 +190,7 @@ func fill(para string, indent int) string {
 
 	var buf bytes.Buffer
 	indentStr := strings.Repeat(" ", indent)
-	doc.ToText(&buf, para, indentStr, indentStr, width-indent)
+	doc.ToText(&buf, para, indentStr, indentStr, width-indent) //lint:ignore SA1019 Deprecated
 
 	return strings.TrimSpace(buf.String())
 }

internals/daemon/api_services.go

@@ -208,13 +208,17 @@ func v1PostServices(c *Command, r *http.Request, _ *UserState) Response {
 		// Can happen with a replan that has no services to stop/start. A
 		// change with no tasks needs to be marked Done manually (normally a
 		// change is marked Done when its last task is finished).
+		//
+		//lint:ignore SA1019 strings.Title is deprecated
 		summary = fmt.Sprintf("%s - no services", strings.Title(payload.Action))
 		change := st.NewChange(payload.Action, summary)
 		change.SetStatus(state.DoneStatus)
 		return AsyncResponse(nil, change.ID())
 	case len(services) == 1:
+		//lint:ignore SA1019 strings.Title is deprecated
 		summary = fmt.Sprintf("%s service %q", strings.Title(payload.Action), payload.Services[0])
 	default:
+		//lint:ignore SA1019 strings.Title is deprecated
 		summary = fmt.Sprintf("%s service %q and %d more", strings.Title(payload.Action), payload.Services[0], len(services)-1)
 	}
 

internals/daemon/daemon.go

@@ -37,7 +37,6 @@ import (
 
 	"github.com/canonical/pebble/internals/logger"
 	"github.com/canonical/pebble/internals/osutil"
-	"github.com/canonical/pebble/internals/osutil/sys"
 	"github.com/canonical/pebble/internals/overlord"
 	"github.com/canonical/pebble/internals/overlord/checkstate"
 	"github.com/canonical/pebble/internals/overlord/restart"
@@ -55,7 +54,6 @@ var (
 	ErrRestartExternal       = fmt.Errorf("daemon stop requested due to externally-handled reboot")
 
 	systemdSdNotify = systemd.SdNotify
-	sysGetuid       = sys.Getuid
 )
 
 // Options holds the daemon setup required for the initialization of a new daemon.
@@ -139,14 +137,6 @@ type Command struct {
 	d *Daemon
 }
 
-type accessResult int
-
-const (
-	accessOK accessResult = iota
-	accessUnauthorized
-	accessForbidden
-)
-
 func userFromRequest(st *state.State, r *http.Request, ucred *Ucrednet, username, password string) (*UserState, error) {
 	var userID *uint32
 	if ucred != nil {
@@ -309,7 +299,7 @@ func logit(handler http.Handler) http.Handler {
 		ww := &wrappedWriter{w: w}
 		t0 := time.Now()
 		handler.ServeHTTP(ww, r)
-		t := time.Now().Sub(t0)
+		t := time.Since(t0)
 
 		// Don't log GET /v1/changes/{change-id} as that's polled quickly by
 		// clients when waiting for a change (e.g., service starting). Also

internals/daemon/daemon_test.go

@@ -53,14 +53,13 @@ import (
 func Test(t *testing.T) { TestingT(t) }
 
 type daemonSuite struct {
-	pebbleDir       string
-	socketPath      string
-	httpAddress     string
-	statePath       string
-	authorized      bool
-	err             error
-	notified        []string
-	restoreBackends func()
+	pebbleDir   string
+	socketPath  string
+	httpAddress string
+	statePath   string
+	authorized  bool
+	err         error
+	notified    []string
 }
 
 var _ = Suite(&daemonSuite{})
@@ -710,7 +709,6 @@ func (s *daemonSuite) TestGracefulStop(c *C) {
 		} else {
 			w.Write([]byte("Gone"))
 		}
-		return
 	})
 
 	generalL, err := net.Listen("tcp", "127.0.0.1:0")
@@ -1567,7 +1565,7 @@ func (s *rebootSuite) TestSyscallPosRebootDelay(c *C) {
 	case <-time.After(10 * time.Second):
 		c.Fatal("syscall did not take place and we timed out")
 	}
-	elapsed := time.Now().Sub(start)
+	elapsed := time.Since(start)
 	c.Assert(elapsed >= period, Equals, true)
 }
 
@@ -1596,7 +1594,7 @@ func (s *rebootSuite) TestSyscallNegRebootDelay(c *C) {
 	case <-time.After(10 * time.Second):
 		c.Fatal("syscall did not take place and we timed out")
 	}
-	elapsed := time.Now().Sub(start)
+	elapsed := time.Since(start)
 	c.Assert(elapsed < period, Equals, true)
 }
 

internals/osutil/export_test.go

@@ -124,3 +124,11 @@ func FakeEnviron(f func() []string) (restore func()) {
 		osEnviron = oldEnviron
 	}
 }
+
+func FakeRandomString(f func(int) string) (restore func()) {
+	old := randomString
+	randomString = f
+	return func() {
+		randomString = old
+	}
+}

internals/osutil/io.go

@@ -43,6 +43,9 @@ const (
 // all unit tests in general.
 var unsafeIO bool = len(os.Args) > 0 && strings.HasSuffix(os.Args[0], ".test") && os.Getenv("UNSAFE_IO") == "1"
 
+// For testing
+var randomString = randutil.RandomString
+
 // An AtomicFile is similar to an os.File but it has an additional
 // Commit() method that does whatever needs to be done so the
 // modification is "atomic": an AtomicFile will do its best to leave
@@ -92,7 +95,7 @@ func NewAtomicFile(filename string, perm os.FileMode, flags AtomicWriteFlags, ui
 	// aa-enforce. Tools from this package enumerate all profiles by loading
 	// parsing any file found in /etc/apparmor.d/, skipping only very specific
 	// suffixes, such as the one we selected below.
-	tmp := filename + "." + randutil.RandomString(12) + "~"
+	tmp := filename + "." + randomString(12) + "~"
 
 	fd, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_EXCL, perm)
 	if err != nil {

internals/osutil/io_test.go

@@ -21,12 +21,11 @@ package osutil_test
 
 import (
 	"errors"
-	"math/rand"
 	"os"
 	"path/filepath"
+	"strings"
 	"syscall"
 
-	"github.com/canonical/x-go/randutil"
 	. "gopkg.in/check.v1"
 
 	"github.com/canonical/pebble/internals/osutil"
@@ -170,14 +169,13 @@ func (ts *AtomicWriteTestSuite) TestAtomicWriteFileOverwriteRelativeSymlink(c *C
 
 func (ts *AtomicWriteTestSuite) TestAtomicWriteFileNoOverwriteTmpExisting(c *C) {
 	tmpdir := c.MkDir()
-	// ensure we always get the same result
-	rand.Seed(1)
-	expectedRandomness := randutil.RandomString(12) + "~"
-	// ensure we always get the same result
-	rand.Seed(1)
+	osutil.FakeRandomString(func(length int) string {
+		// ensure we always get the same result
+		return strings.Repeat("a", length)
+	})
 
 	p := filepath.Join(tmpdir, "foo")
-	err := os.WriteFile(p+"."+expectedRandomness, []byte(""), 0644)
+	err := os.WriteFile(p+"."+strings.Repeat("a", 12)+"~", []byte(""), 0644)
 	c.Assert(err, IsNil)
 
 	err = osutil.AtomicWriteFile(p, []byte(""), 0600, 0)

internals/osutil/squashfs/fstype.go

@@ -39,7 +39,7 @@ func useFuseImpl() bool {
 	}
 
 	virt := strings.TrimSpace(string(out))
-	if virt != "none" {
+	if virt != "none" { // lint:ignore S1008 Should use 'return cond'
 		return true
 	}
 

internals/osutil/user_test.go

@@ -29,8 +29,6 @@ import (
 
 type userSuite struct {
 	testutil.BaseTest
-
-	restorer func()
 }
 
 var _ = check.Suite(&userSuite{})

internals/overlord/checkstate/checkers.go

@@ -52,6 +52,9 @@ func (c *httpChecker) check(ctx context.Context) error {
 	logger.Debugf("Check %q (http): requesting %q", c.name, c.url)
 	client := &http.Client{}
 	request, err := http.NewRequestWithContext(ctx, "GET", c.url, nil)
+	if err != nil {
+		return fmt.Errorf("cannot build request: %w", err)
+	}
 	for k, v := range c.headers {
 		request.Header.Set(k, v)
 	}

internals/overlord/checkstate/checkers_test.go

@@ -112,6 +112,11 @@ func (s *CheckersSuite) TestHTTP(c *C) {
 	chk = &httpChecker{url: server.URL}
 	err = chk.check(context.Background())
 	c.Assert(err, ErrorMatches, ".* connection refused")
+
+	// Malformed URL returns an error
+	chk = &httpChecker{url: "#!@$%@#@"}
+	err = chk.check(ctx)
+	c.Assert(err, ErrorMatches, "cannot build request: .*")
 }
 
 func (s *CheckersSuite) TestTCP(c *C) {

internals/overlord/checkstate/manager_test.go

@@ -585,7 +585,6 @@ func waitChecks(c *C, mgr *checkstate.CheckManager, expected []*checkstate.Check
 		c.Logf("check %d: %#v", i, *check)
 	}
 	c.Fatal("timed out waiting for checks to settle")
-	return
 }
 
 func lastTaskLog(st *state.State, changeID string) string {

internals/overlord/managers_test.go

@@ -17,8 +17,6 @@ package overlord_test
 // test the various managers and their operation together through overlord
 
 import (
-	"time"
-
 	. "gopkg.in/check.v1"
 
 	"github.com/canonical/pebble/internals/overlord"
@@ -48,5 +46,3 @@ func (s *mgrsSuite) SetUpTest(c *C) {
 	c.Assert(err, IsNil)
 	s.o = o
 }
-
-var settleTimeout = 15 * time.Second

internals/overlord/overlord_test.go

@@ -155,7 +155,7 @@ func (ovs *overlordSuite) TestNewWithPatches(c *C) {
 	}
 	patch.Fake(1, 1, map[int][]patch.PatchFunc{1: {p, sp}})
 
-	fakeState := []byte(fmt.Sprintf(`{"data":{"patch-level":0, "patch-sublevel":0}}`))
+	fakeState := []byte(`{"data":{"patch-level":0, "patch-sublevel":0}}`)
 	err := os.WriteFile(ovs.statePath, fakeState, 0600)
 	c.Assert(err, IsNil)