Merge pull request #31 from ngrok-oss/euan/maintainership-update-some…

…-stuff

Upgrade to go1.21 and fix various lints

.envrc

@@ -0,0 +1,7 @@
+if ! has nix_direnv_version || ! nix_direnv_version 2.2.0; then
+  source_url "https://raw.githubusercontent.com/nix-community/nix-direnv/2.2.0/direnvrc" "sha256-5EwyKnkJNQeXrRkYbwwRBcXbibosCJqyIUuz9Xq+LRc="
+fi
+dotenv_if_exists
+GOPATH=`pwd`/.direnv/go
+PATH=${GOPATH}/bin:${PATH}
+use flake

.github/workflows/lint.yml

@@ -0,0 +1,32 @@
+name: lint
+on:
+  push:
+    branches: [ "main" ]
+  pull_request:
+    branches: [ "main" ]
+permissions:
+  contents: read
+  # Optional: allow read access to pull request. Use with `only-new-issues` option.
+  # pull-requests: read
+jobs:
+  lint:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 2
+      - uses: cachix/install-nix-action@v18
+      - uses: HatsuneMiku3939/direnv-action@v1
+      - name: direnv allow
+        run: direnv allow .
+      - name: Run goimports
+        shell: bash
+        run: |
+          shopt -s globstar
+          direnv exec . goimports -format-only -w -local github.com/ngrok/tableroll **/*.go
+      - name: Lint
+        run: direnv exec . golangci-lint run .
+      - name: Check diff
+        shell: bash
+        run: git diff --exit-code

.github/workflows/test.yml

@@ -0,0 +1,22 @@
+name: Go
+on:
+  push:
+    branches: [ "main" ]
+  pull_request:
+    branches: [ "main" ]
+jobs:
+  build-and-test:
+    runs-on: ubuntu-latest
+    env:
+      NGROK_TEST_ONLINE: 1
+      NGROK_TEST_LONG: 1
+      NGROK_TEST_FLAKEY: 1
+      NGROK_AUTHTOKEN: ${{ secrets.NGROK_AUTHTOKEN }}
+    steps:
+    - uses: actions/checkout@v3
+    - uses: cachix/install-nix-action@v18
+    - uses: HatsuneMiku3939/direnv-action@v1
+    - name: direnv allow
+      run: direnv allow .
+    - name: Test
+      run: direnv exec . go test -v ./...

.gitignore

@@ -0,0 +1 @@
+/.direnv

coordination.go

@@ -3,7 +3,6 @@ package tableroll
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
 	"net"
 	"os"
 	"path/filepath"
@@ -52,7 +51,7 @@ func (c *coordinator) Listen(ctx context.Context) (*net.UnixListener, error) {
 }
 
 func touchFile(path string) error {
-	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDONLY, 0755)
+	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDONLY, 0o755)
 	f.Close()
 	return err
 }
@@ -96,7 +95,7 @@ func (c *coordinator) idFile() string {
 // It should only be called while the lock is held.
 func (c *coordinator) BecomeOwner() error {
 	c.l.Info("writing id to become owner", "id", c.id)
-	return ioutil.WriteFile(c.idFile(), []byte(c.id), 0755)
+	return os.WriteFile(c.idFile(), []byte(c.id), 0o755)
 }
 
 // Unlock unlocks the coordination id file
@@ -113,7 +112,7 @@ func (c *coordinator) Unlock() error {
 // It will return 'errNoOwner' if there isn't currently an owner.
 func (c *coordinator) GetOwnerID() (string, error) {
 	c.l.Info("discovering current owner")
-	data, err := ioutil.ReadFile(c.idFile())
+	data, err := os.ReadFile(c.idFile())
 	if err != nil {
 		return "", err
 	}

coordinator_test.go

@@ -2,75 +2,63 @@ package tableroll
 
 import (
 	"context"
-	"io/ioutil"
-	"os"
+	"io"
 	"testing"
 
 	"github.com/inconshreveable/log15"
+	"github.com/stretchr/testify/require"
 	"k8s.io/utils/clock"
 )
 
 // TestConnectOwner is a happy-path test of using the coordinator
 func TestConnectOwner(t *testing.T) {
 	l := log15.New()
 	ctx := context.Background()
-	tmpdir, err := ioutil.TempDir("", "tableroll_coord_test")
-	if err != nil {
-		panic(err)
-	}
-	defer os.RemoveAll(tmpdir)
+	tmpdir := tmpDir(t)
 
 	coord1 := newCoordinator(clock.RealClock{}, l, tmpdir, "1")
 	coord2 := newCoordinator(clock.RealClock{}, l, tmpdir, "2")
 
 	coord1l, err := coord1.Listen(ctx)
-	if err != nil {
-		t.Fatalf("Unable to listen: %v", err)
-	}
-	coord1.Lock(ctx)
-	coord1.BecomeOwner()
-	coord1.Unlock()
+	require.NoError(t, err)
+	require.NoError(t, coord1.Lock(ctx))
+	require.NoError(t, coord1.BecomeOwner())
+	require.NoError(t, coord1.Unlock())
 
 	connw, err := coord2.ConnectOwner(ctx)
-	if err != nil {
-		t.Fatalf("unable to connect to owner")
-	}
+	require.NoError(t, err)
 
+	connErr := make(chan error, 1)
 	go func() {
-		connw.Write([]byte("hello world"))
-		connw.Close()
+		defer close(connErr)
+		if _, err := connw.Write([]byte("hello world")); err != nil {
+			connErr <- err
+		}
+		if err := connw.Close(); err != nil {
+			connErr <- err
+		}
 	}()
 
 	connr, err := coord1l.Accept()
 	if err != nil {
 		t.Fatalf("acccept err: %v", err)
 	}
-	data, err := ioutil.ReadAll(connr)
-	if err != nil {
-		t.Fatalf("read err: %v", err)
-	}
-	if string(data) != "hello world" {
-		t.Fatalf("expected to read %q; got %q", "hello world", string(data))
-	}
+	data, err := io.ReadAll(connr)
+	require.NoError(t, err)
+	require.Equal(t, "hello world", string(data))
+	require.NoError(t, <-connErr)
 }
 
 // TestLockCoordinationDirCtxCancel tests that a call to `lockCoordinationDir` can be
 // canceled by canceling the passed in context.
 func TestLockCoordinationDirCtxCancel(t *testing.T) {
 	l := log15.New()
-	ctx := context.Background()
-	tmpdir, err := ioutil.TempDir("", "tableroll_coord_test")
-	if err != nil {
-		panic(err)
-	}
-	defer os.RemoveAll(tmpdir)
+	ctx := testCtx(t)
+	tmpdir := tmpDir(t)
 	coord1 := newCoordinator(clock.RealClock{}, l, tmpdir, "1")
 	coord2 := newCoordinator(clock.RealClock{}, l, tmpdir, "2")
-	err = coord1.Lock(ctx)
-	if err != nil {
-		t.Fatalf("Error getting coordination dir: %v", err)
-	}
-	defer coord1.Unlock()
+	require.NoError(t, coord1.Lock(ctx))
+	defer func() { require.NoError(t, coord1.Unlock()) }()
 
 	ctx2, cancel := context.WithCancel(ctx)
 	coordErr := make(chan error)
@@ -85,7 +73,7 @@ func TestLockCoordinationDirCtxCancel(t *testing.T) {
 	default:
 	}
 	cancel()
-	err = <-coordErr
+	err := <-coordErr
 	if err != context.Canceled {
 		t.Errorf("expected context cancel, got %v", err)
 	}

dup_file.go

@@ -1,3 +1,4 @@
+//go:build go1.12
 // +build go1.12
 
 package tableroll

dup_file_legacy.go

@@ -1,3 +1,4 @@
+//go:build !go1.12
 // +build !go1.12
 
 package tableroll

fds.go

@@ -48,8 +48,8 @@ type fdKind string
 
 const (
 	fdKindListener fdKind = "listener"
-	fdKindConn            = "conn"
-	fdKindFile            = "file"
+	fdKindConn     fdKind = "conn"
+	fdKindFile     fdKind = "file"
 )
 
 // file works around the fact that it's not possible
@@ -93,8 +93,8 @@ type fd struct {
 	ID string `json:"id"`
 
 	// for conns/listeners, stored just for pretty-printing
-	Network string `json:"network,omitEmpty"`
-	Addr    string `json:"addr,omitEmpty"`
+	Network string `json:"network,omitempty"`
+	Addr    string `json:"addr,omitempty"`
 }
 
 func (f *fd) String() string {
@@ -449,19 +449,6 @@ func (f *Fds) copy() map[string]*fd {
 	return files
 }
 
-func unlinkUnixSocket(path string) error {
-	info, err := os.Stat(path)
-	if err != nil {
-		return err
-	}
-
-	if info.Mode()&os.ModeSocket == 0 {
-		return nil
-	}
-
-	return os.Remove(path)
-}
-
 func dupConn(conn syscall.Conn, name string) (*file, error) {
 	// Use SyscallConn instead of File to avoid making the original
 	// fd non-blocking.

fds_test.go

@@ -2,11 +2,12 @@ package tableroll
 
 import (
 	"context"
-	"io/ioutil"
 	"net"
 	"os"
 	"path/filepath"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestFdsListen(t *testing.T) {
@@ -42,11 +43,7 @@ func TestFdsListener(t *testing.T) {
 	}
 	defer tcp.Close()
 
-	temp, err := ioutil.TempDir("", "tableflip")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(temp)
+	temp := tmpDir(t)
 
 	socketPath := filepath.Join(temp, "socket")
 	socketPath2 := filepath.Join(temp, "socket2")
@@ -84,20 +81,19 @@ func TestFdsListener(t *testing.T) {
 
 	child := newFds(l, parent.copy())
 	ln, err := child.Listener("1")
-	if err != nil {
-		t.Fatal("Can't get listener:", err)
-	}
+	require.NoError(t, err)
 	if ln == nil {
 		t.Fatal("Missing listener")
 	}
 	ln.Close()
 
-	child.Remove("2")
+	require.NoError(t, child.Remove("2"))
 	if _, err := os.Stat(socketPath); err != nil {
 		t.Errorf("expected socket to still exist")
 	}
 
 	ln, err = child.Listener("3")
+	require.NoError(t, err)
 	ln.(*net.UnixListener).SetUnlinkOnClose(true)
 	ln.Close()
 	if _, err := os.Stat(socketPath2); err == nil {
@@ -117,10 +113,10 @@ func TestFdsConn(t *testing.T) {
 		t.Fatal("Can't add conn:", err)
 	}
 	unixConn.Close()
-	defer parent.Remove("1")
+	defer func() { _ = parent.Remove("1") }()
 
 	child := newFds(l, parent.copy())
-	defer child.Remove("1")
+	defer func() { _ = child.Remove("1") }()
 	conn, err := child.Conn("1")
 	if err != nil {
 		t.Fatal("Can't get conn:", err)
@@ -145,7 +141,7 @@ func TestFdsFile(t *testing.T) {
 		t.Fatal("Can't add file:", err)
 	}
 	w.Close()
-	defer parent.Remove("test")
+	defer func() { require.NoError(t, parent.Remove("test")) }()
 
 	child := newFds(l, parent.copy())
 	file, err := child.File("test")
@@ -162,7 +158,7 @@ func TestFdsLock(t *testing.T) {
 	fds := newFds(l, nil)
 
 	ln, err := fds.ListenWith("1", "tcp", "127.0.0.1:0", net.Listen)
-	defer ln.Close()
+	defer func() { require.NoError(t, ln.Close()) }()
 	if err != nil {
 		t.Fatalf("expected no error in unlocked fds: %v", err)
 	}