Merge branch '797-console-lint' into 'main'

Lint fixes for console executable and gitaly packages

Closes #797

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/1154



Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Reviewed-by: Ash McKenzie <amckenzie@gitlab.com>
Co-authored-by: Archish <archishthakkar@gmail.com>

internal/console/console.go

@@ -36,7 +36,7 @@ func DisplayMessages(messages []string, out io.Writer, displayDivider bool) {
 	displayBlankLineOrDivider(out, displayDivider)
 
 	for _, msg := range messages {
-		fmt.Fprint(out, formatLine(msg))
+		_, _ = fmt.Fprint(out, formatLine(msg))
 	}
 
 	displayBlankLineOrDivider(out, displayDivider)
@@ -62,9 +62,9 @@ func formatLine(message string) string {
 
 func displayBlankLineOrDivider(out io.Writer, displayDivider bool) {
 	if displayDivider {
-		fmt.Fprint(out, divider())
+		_, _ = fmt.Fprint(out, divider())
 	} else {
-		fmt.Fprint(out, blankLine())
+		_, _ = fmt.Fprint(out, blankLine())
 	}
 }
 

internal/executable/executable.go

+// Package executable provides utilities for managing and locating executables related to GitLab Shell.
 package executable
 
 import (
@@ -5,6 +6,7 @@ import (
 	"path/filepath"
 )
 
+// Predefined constants representing various executable names and directories.
 const (
 	BinDir                    = "bin"
 	Healthcheck               = "check"
@@ -13,6 +15,7 @@ const (
 	AuthorizedPrincipalsCheck = "gitlab-shell-authorized-principals-check"
 )
 
+// Executable represents an executable binary, including its name and root directory.
 type Executable struct {
 	Name    string
 	RootDir string
@@ -23,6 +26,7 @@ var (
 	osExecutable = os.Executable
 )
 
+// New creates a new Executable instance by determining its root directory based on the current executable path.
 func New(name string) (*Executable, error) {
 	path, err := osExecutable()
 	if err != nil {

internal/executable/executable_test.go

@@ -61,8 +61,8 @@ func TestNewSuccess(t *testing.T) {
 			result, err := New("gitlab-shell")
 
 			require.NoError(t, err)
-			require.Equal(t, result.Name, "gitlab-shell")
-			require.Equal(t, result.RootDir, tc.expectedRootDir)
+			require.Equal(t, "gitlab-shell", result.Name)
+			require.Equal(t, tc.expectedRootDir, result.RootDir)
 		})
 	}
 }

internal/gitaly/gitaly.go

+// Package gitaly provides a client for interacting with Gitaly services over gRPC.
 package gitaly
 
 import (
@@ -9,7 +10,6 @@ import (
 	"google.golang.org/grpc"
 
 	gitalyauth "gitlab.com/gitlab-org/gitaly/v16/auth"
-	"gitlab.com/gitlab-org/gitaly/v16/client"
 	gitalyclient "gitlab.com/gitlab-org/gitaly/v16/client"
 	"gitlab.com/gitlab-org/labkit/correlation"
 	grpccorrelation "gitlab.com/gitlab-org/labkit/correlation/grpc"
@@ -19,6 +19,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/metrics"
 )
 
+// Command represents a gRPC service command with its address and token.
 type Command struct {
 	ServiceName string
 	Address     string
@@ -31,33 +32,36 @@ type connectionsCache struct {
 	connections map[Command]*grpc.ClientConn
 }
 
+// Client manages connections to Gitaly services and handles sidechannel communication.
 type Client struct {
 	SidechannelRegistry *gitalyclient.SidechannelRegistry
 
 	cache connectionsCache
 }
 
+// InitSidechannelRegistry initializes the sidechannel registry for gRPC connections.
 func (c *Client) InitSidechannelRegistry(ctx context.Context) {
 	c.SidechannelRegistry = gitalyclient.NewSidechannelRegistry(log.ContextLogger(ctx))
 }
 
+// GetConnection returns a gRPC connection for the given command, using a cached connection if available.
 func (c *Client) GetConnection(ctx context.Context, cmd Command) (*grpc.ClientConn, error) {
 	c.cache.RLock()
-	conn := c.cache.connections[cmd]
+	existingConn := c.cache.connections[cmd]
 	c.cache.RUnlock()
 
-	if conn != nil {
-		return conn, nil
+	if existingConn != nil {
+		return existingConn, nil
 	}
 
 	c.cache.Lock()
 	defer c.cache.Unlock()
 
-	if conn := c.cache.connections[cmd]; conn != nil {
-		return conn, nil
+	if cachedConn := c.cache.connections[cmd]; cachedConn != nil {
+		return cachedConn, nil
 	}
 
-	conn, err := c.newConnection(ctx, cmd)
+	newConn, err := c.newConnection(ctx, cmd)
 	if err != nil {
 		return nil, err
 	}
@@ -66,9 +70,9 @@ func (c *Client) GetConnection(ctx context.Context, cmd Command) (*grpc.ClientCo
 		c.cache.connections = make(map[Command]*grpc.ClientConn)
 	}
 
-	c.cache.connections[cmd] = conn
+	c.cache.connections[cmd] = newConn
 
-	return conn, nil
+	return newConn, nil
 }
 
 func (c *Client) newConnection(ctx context.Context, cmd Command) (conn *grpc.ClientConn, err error) {
@@ -93,7 +97,7 @@ func (c *Client) newConnection(ctx context.Context, cmd Command) (conn *grpc.Cli
 
 	serviceName = fmt.Sprintf("%s-%s", serviceName, cmd.ServiceName)
 
-	connOpts := client.DefaultDialOpts
+	connOpts := gitalyclient.DefaultDialOpts
 	connOpts = append(
 		connOpts,
 		grpc.WithChainStreamInterceptor(
@@ -129,5 +133,5 @@ func (c *Client) newConnection(ctx context.Context, cmd Command) (conn *grpc.Cli
 		)
 	}
 
-	return client.DialSidechannel(ctx, cmd.Address, c.SidechannelRegistry, connOpts)
+	return gitalyclient.DialSidechannel(ctx, cmd.Address, c.SidechannelRegistry, connOpts)
 }

support/lint_last_known_acceptable.txt

@@ -112,23 +112,6 @@ internal/config/config.go:196:9: copylocks: assignment copies lock value to *cfg
 internal/config/config.go:199:22: G304: Potential file inclusion via variable (gosec)
 internal/config/config.go:210:3: var-naming: var unescapedUrl should be unescapedURL (revive)
 internal/config/config.go:256:1: receiver-naming: receiver name cfg should be consistent with previous receiver name c for Config (revive)
-internal/console/console.go:39:13: Error return value of `fmt.Fprint` is not checked (errcheck)
-internal/console/console.go:65:13: Error return value of `fmt.Fprint` is not checked (errcheck)
-internal/console/console.go:67:13: Error return value of `fmt.Fprint` is not checked (errcheck)
-internal/executable/executable.go:1:1: package-comments: should have a package comment (revive)
-internal/executable/executable.go:9:2: exported: exported const BinDir should have comment (or a comment on this block) or be unexported (revive)
-internal/executable/executable.go:16:6: exported: exported type Executable should have comment or be unexported (revive)
-internal/executable/executable.go:26:1: exported: exported function New should have comment or be unexported (revive)
-internal/executable/executable_test.go:64:4: expected-actual: need to reverse actual and expected values (testifylint)
-internal/executable/executable_test.go:65:4: expected-actual: need to reverse actual and expected values (testifylint)
-internal/gitaly/gitaly.go:1:1: package-comments: should have a package comment (revive)
-internal/gitaly/gitaly.go:12:2: ST1019: package "gitlab.com/gitlab-org/gitaly/v16/client" is being imported more than once (stylecheck)
-internal/gitaly/gitaly.go:13:2: ST1019(related information): other import of "gitlab.com/gitlab-org/gitaly/v16/client" (stylecheck)
-internal/gitaly/gitaly.go:22:6: exported: exported type Command should have comment or be unexported (revive)
-internal/gitaly/gitaly.go:34:6: exported: exported type Client should have comment or be unexported (revive)
-internal/gitaly/gitaly.go:40:1: exported: exported method Client.InitSidechannelRegistry should have comment or be unexported (revive)
-internal/gitaly/gitaly.go:44:1: exported: exported method Client.GetConnection should have comment or be unexported (revive)
-internal/gitaly/gitaly.go:56:5: shadow: declaration of "conn" shadows declaration at line 46 (govet)
 internal/gitlabnet/client.go:1:1: package-comments: should have a package comment (revive)
 internal/gitlabnet/client.go:14:1: exported: exported function GetClient should have comment or be unexported (revive)
 internal/gitlabnet/client.go:21:15: ST1005: error strings should not be capitalized (stylecheck)