Merge pull request juju#16899 from SimonRichardson/lint-against-expor…

…ted-unexported-values

juju#16899

This is the first round of linter rules added to the code base to improve the code base at large.

The following linter rules that I added:

 - unexported-naming
 - exported
 - atomic
 - defer

I really wanted to add "loop" argument to defer, but unfortunately, the fallout requires a lot more time and effort to undo some of the problems.

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing

## QA steps

```sh
$ juju bootstrap lxd test
```

Also, all the tests should pass.


## Links

**Jira card:** JUJU-

.github/golangci-lint.config.yaml

@@ -29,6 +29,25 @@ linters-settings:
       - unreachable
       - unsafeptr
       - unusedresult
+  revive:
+    rules:
+      - name: unexported-naming
+        severity: error
+        disabled: false
+      - name: exported
+        severity: error
+        disabled: false
+        arguments:
+          - "disableStutteringCheck"
+          - "sayRepetitiveInsteadOfStutters"
+      - name: atomic
+        severity: error
+        disabled: false
+      - name: defer
+        severity: error
+        disabled: false
+        arguments:
+          - [ "return", "recover", "method-call" ]
 linters:
   disable-all: true
   enable:
@@ -41,6 +60,7 @@ linters:
     - exportloopref
     - unused
     - sqlclosecheck
+    - revive
 run:
   timeout: 30m
   modules-download-mode: readonly

agent/agentbootstrap/bootstrap_test.go

@@ -127,7 +127,7 @@ func (s *bootstrapSuite) TestInitializeState(c *gc.C) {
 	controllerCfg := testing.FakeControllerConfig()
 
 	initialModelUUID := utils.MustNewUUID().String()
-	InitialModelConfigAttrs := map[string]interface{}{
+	initialModelConfigAttrs := map[string]interface{}{
 		"name": "hosted",
 		"uuid": initialModelUUID,
 	}
@@ -160,7 +160,7 @@ func (s *bootstrapSuite) TestInitializeState(c *gc.C) {
 		ControllerModelEnvironVersion: 666,
 		ModelConstraints:              expectModelConstraints,
 		ControllerInheritedConfig:     controllerInheritedConfig,
-		InitialModelConfig:            InitialModelConfigAttrs,
+		InitialModelConfig:            initialModelConfigAttrs,
 		StoragePools: map[string]storage.Attrs{
 			"spool": {
 				"type": "loop",
@@ -424,7 +424,7 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) {
 	modelCfg, err := config.New(config.NoDefaults, modelAttrs)
 	c.Assert(err, jc.ErrorIsNil)
 
-	InitialModelConfigAttrs := map[string]interface{}{
+	initialModelConfigAttrs := map[string]interface{}{
 		"name": "hosted",
 		"uuid": utils.MustNewUUID().String(),
 	}
@@ -440,7 +440,7 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) {
 		},
 		ControllerConfig:      testing.FakeControllerConfig(),
 		ControllerModelConfig: modelCfg,
-		InitialModelConfig:    InitialModelConfigAttrs,
+		InitialModelConfig:    initialModelConfigAttrs,
 	}
 
 	adminUser := names.NewLocalUserTag("agent-admin")

api/client/action/client.go

@@ -67,9 +67,9 @@ func (c *Client) ListOperations(arg OperationQueryArgs) (Operations, error) {
 }
 
 // Operation fetches the operation with the specified ID.
-func (c *Client) Operation(ID string) (Operation, error) {
+func (c *Client) Operation(id string) (Operation, error) {
 	arg := params.Entities{
-		Entities: []params.Entity{{names.NewOperationTag(ID).String()}},
+		Entities: []params.Entity{{Tag: names.NewOperationTag(id).String()}},
 	}
 	var results params.OperationResults
 	err := c.facade.FacadeCall(context.TODO(), "Operations", arg, &results)

apiserver/apiserverhttp/mux_test.go

@@ -92,19 +92,21 @@ func (s *MuxSuite) TestConcurrentAddHandler(c *gc.C) {
 	// Concurrently add and remove another handler to show that
 	// adding and removing handlers will not race with request
 	// handling.
-	const N = 1000
+
+	// bN is the number of add and remove handlers to make.
+	const bN = 1000
 	var wg sync.WaitGroup
 	wg.Add(1)
 	go func() {
 		defer wg.Done()
-		for i := 0; i < N; i++ {
+		for i := 0; i < bN; i++ {
 			s.mux.AddHandler("POST", "/", http.NotFoundHandler())
 			s.mux.RemoveHandler("POST", "/")
 		}
 	}()
 	defer wg.Wait()
 
-	for i := 0; i < N; i++ {
+	for i := 0; i < bN; i++ {
 		resp, err := s.client.Get(s.server.URL + "/")
 		c.Assert(err, jc.ErrorIsNil)
 		resp.Body.Close()
@@ -118,14 +120,16 @@ func (s *MuxSuite) TestConcurrentRemoveHandler(c *gc.C) {
 	// Concurrently add and remove another handler to show that
 	// adding and removing handlers will not race with request
 	// handling.
-	const N = 500
+
+	// bN is the number of add and remove handlers to make.
+	const bN = 500
 	var wg sync.WaitGroup
 	wg.Add(1)
 	done := make(chan struct{})
 	go func() {
 		defer wg.Done()
 		defer close(done)
-		for i := 0; i < N; i++ {
+		for i := 0; i < bN; i++ {
 			s.mux.AddHandler("GET", "/", h)
 			// Sleep to give the client a
 			// chance to hit the endpoint.
@@ -140,7 +144,7 @@ func (s *MuxSuite) TestConcurrentRemoveHandler(c *gc.C) {
 out:
 	for {
 		select {
-		case _, _ = <-done:
+		case <-done:
 			break out
 		default:
 		}

apiserver/authentication/jwt/package_test.go

@@ -4,7 +4,7 @@
 package jwt_test
 
 import (
-	. "testing"
+	"testing"
 	"time"
 
 	"github.com/google/uuid"
@@ -15,7 +15,7 @@ import (
 	gc "gopkg.in/check.v1"
 )
 
-func TestPackage(t *T) {
+func TestPackage(t *testing.T) {
 	gc.TestingT(t)
 }
 

apiserver/facades/agent/caasapplication/state.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/juju/juju/caas"
 	"github.com/juju/juju/controller"
-	jujucontroller "github.com/juju/juju/controller"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/state"
 )
@@ -26,7 +25,7 @@ type State interface {
 // ControllerState provides the subset of controller state
 // required by the CAAS application facade.
 type ControllerState interface {
-	ControllerConfig() (jujucontroller.Config, error)
+	ControllerConfig() (controller.Config, error)
 	APIHostPortsForAgents(controller.Config) ([]network.SpaceHostPorts, error)
 }
 

apiserver/facades/agent/secretsmanager/secrets.go

@@ -229,8 +229,8 @@ func (s *SecretsManagerAPI) CreateSecrets(ctx context.Context, args params.Creat
 		Results: make([]params.StringResult, len(args.Args)),
 	}
 	for i, arg := range args.Args {
-		ID, err := s.createSecret(arg)
-		result.Results[i].Result = ID
+		id, err := s.createSecret(arg)
+		result.Results[i].Result = id
 		if errors.Is(err, state.LabelExists) {
 			err = errors.AlreadyExistsf("secret with label %q", *arg.Label)
 		}

apiserver/facades/agent/uniter/networkinfoiaas.go

@@ -480,18 +480,15 @@ func (n *NetworkInfoIAAS) networkInfoForSpace(spaceID string) params.NetworkInfo
 		}
 
 		// Otherwise, add a new device.
-		var MAC string
 		mac, err := addr.HWAddr()
-		if err == nil {
-			MAC = mac
-		} else if !errors.Is(err, errors.NotFound) {
+		if err != nil && !errors.Is(err, errors.NotFound) {
 			res.Error = apiservererrors.ServerError(err)
 			return res
 		}
 
 		res.Info = append(res.Info, params.NetworkInfo{
 			InterfaceName: addr.DeviceName(),
-			MACAddress:    MAC,
+			MACAddress:    mac,
 			Addresses:     []params.InterfaceAddress{deviceAddr},
 		})
 	}

apiserver/facades/client/annotations/client_test.go

@@ -16,15 +16,14 @@ import (
 	apiservertesting "github.com/juju/juju/apiserver/testing"
 	"github.com/juju/juju/core/objectstore"
 	"github.com/juju/juju/juju/testing"
-	jujutesting "github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/testing/factory"
 )
 
 type annotationSuite struct {
 	// TODO(anastasiamac) mock to remove ApiServerSuite
-	jujutesting.ApiServerSuite
+	testing.ApiServerSuite
 
 	annotationsAPI *annotations.API
 	authorizer     apiservertesting.FakeAuthorizer
@@ -37,7 +36,7 @@ var _ = gc.Suite(&annotationSuite{})
 func (s *annotationSuite) SetUpTest(c *gc.C) {
 	s.ApiServerSuite.SetUpTest(c)
 	s.authorizer = apiservertesting.FakeAuthorizer{
-		Tag: jujutesting.AdminUser,
+		Tag: testing.AdminUser,
 	}
 	var err error
 	s.annotationsAPI, err = annotations.NewAPI(facadetest.Context{

apiserver/facades/client/bundle/bundle.go

@@ -621,14 +621,14 @@ func (b *BundleAPI) bundleDataMachines(machines []description.Machine, machineId
 }
 
 func bundleDataRemoteApplications(remoteApps []description.RemoteApplication) map[string]*charm.SaasSpec {
-	Saas := make(map[string]*charm.SaasSpec, len(remoteApps))
+	saas := make(map[string]*charm.SaasSpec, len(remoteApps))
 	for _, application := range remoteApps {
 		newSaas := &charm.SaasSpec{
 			URL: application.URL(),
 		}
-		Saas[application.Name()] = newSaas
+		saas[application.Name()] = newSaas
 	}
-	return Saas
+	return saas
 }
 
 func bundleDataRelations(relations []description.Relation) [][]string {

apiserver/facades/client/client/status_test.go

@@ -240,13 +240,13 @@ func (s *statusSuite) TestFullStatusInterfaceScaling(c *gc.C) {
 			"in the way the addresses are processed"))
 }
 
-func (s *statusSuite) createSpaceAndSubnetWithProviderID(c *gc.C, spaceName, CIDR, providerSubnetID string) {
+func (s *statusSuite) createSpaceAndSubnetWithProviderID(c *gc.C, spaceName, cidr, providerSubnetID string) {
 	st := s.ControllerModel(c).State()
 	space, err := st.AddSpace(spaceName, network.Id(spaceName), nil)
 	c.Assert(err, jc.ErrorIsNil)
 
 	_, err = st.AddSubnet(network.SubnetInfo{
-		CIDR:       CIDR,
+		CIDR:       cidr,
 		SpaceID:    space.Id(),
 		ProviderId: network.Id(providerSubnetID),
 	})

apiserver/facades/client/resources/facade.go

@@ -225,14 +225,14 @@ func (a *API) addPendingResource(appName string, chRes charmresource.Resource) (
 }
 
 func parseApplicationTag(tagStr string) (names.ApplicationTag, *params.Error) { // note the concrete error type
-	ApplicationTag, err := names.ParseApplicationTag(tagStr)
+	applicationTag, err := names.ParseApplicationTag(tagStr)
 	if err != nil {
-		return ApplicationTag, &params.Error{
+		return applicationTag, &params.Error{
 			Message: err.Error(),
 			Code:    params.CodeBadRequest,
 		}
 	}
-	return ApplicationTag, nil
+	return applicationTag, nil
 }
 
 func errorResult(err error) params.ResourcesResult {

apiserver/facades/client/secrets/secrets.go

@@ -268,8 +268,8 @@ func (s *SecretsAPI) CreateSecrets(ctx context.Context, args params.CreateSecret
 		return result, errors.Trace(err)
 	}
 	for i, arg := range args.Args {
-		ID, err := s.createSecret(ctx, backend, arg)
-		result.Results[i].Result = ID
+		id, err := s.createSecret(ctx, backend, arg)
+		result.Results[i].Result = id
 		if errors.Is(err, state.LabelExists) {
 			err = errors.AlreadyExistsf("secret with name %q", *arg.Label)
 		}

apiserver/facades/controller/applicationscaler/facade.go

@@ -79,9 +79,9 @@ func (facade *Facade) rescaleOne(tagString string) error {
 	if err != nil {
 		return errors.Trace(err)
 	}
-	ApplicationTag, ok := tag.(names.ApplicationTag)
+	applicationTag, ok := tag.(names.ApplicationTag)
 	if !ok {
 		return apiservererrors.ErrPerm
 	}
-	return facade.backend.RescaleService(ApplicationTag.Id())
+	return facade.backend.RescaleService(applicationTag.Id())
 }

apiserver/facades/controller/instancepoller/instancepoller.go

@@ -268,10 +268,10 @@ func mapNetworkConfigsToProviderAddresses(
 }
 
 func spaceInfoForAddress(
-	spaceInfos network.SpaceInfos, CIDR, providerSubnetID, addr string,
+	spaceInfos network.SpaceInfos, cidr, providerSubnetID, addr string,
 ) (*network.SpaceInfo, error) {
-	if CIDR != "" {
-		return spaceInfos.InferSpaceFromCIDRAndSubnetID(CIDR, providerSubnetID)
+	if cidr != "" {
+		return spaceInfos.InferSpaceFromCIDRAndSubnetID(cidr, providerSubnetID)
 	}
 	return spaceInfos.InferSpaceFromAddress(addr)
 }

apiserver/logsink/logsink.go

@@ -246,8 +246,8 @@ func (h *logSinkHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 			case m, ok := <-logCh:
 				if !ok {
 					h.mu.Lock()
-					defer h.mu.Unlock()
 					h.receiverStopped = true
+					h.mu.Unlock()
 					return
 				}
 
@@ -330,8 +330,8 @@ func (h *logSinkHandler) receiveLogs(socket *websocket.Conn,
 				// has already disconnected from us, this will fail, but we don't
 				// care that much.
 				h.mu.Lock()
-				defer h.mu.Unlock()
 				_ = socket.WriteMessage(gorillaws.CloseMessage, []byte{})
+				h.mu.Unlock()
 				return
 			}
 			h.metrics.LogReadCount(resolvedModelUUID, metricLogReadLabelSuccess).Inc()

apiserver/logstream_test.go

@@ -244,10 +244,10 @@ func (s *LogStreamIntSuite) TestFullRequest(c *gc.C) {
 func (s *LogStreamIntSuite) newReq(c *gc.C, cfg params.LogStreamConfig) *http.Request {
 	attrs, err := query.Values(cfg)
 	c.Assert(err, jc.ErrorIsNil)
-	URL, err := url.Parse("https://a.b.c/logstream")
+	u, err := url.Parse("https://a.b.c/logstream")
 	c.Assert(err, jc.ErrorIsNil)
-	URL.RawQuery = attrs.Encode()
-	req, err := http.NewRequest("GET", URL.String(), nil)
+	u.RawQuery = attrs.Encode()
+	req, err := http.NewRequest("GET", u.String(), nil)
 	c.Assert(err, jc.ErrorIsNil)
 	return req
 }

apiserver/testing/fakeapi.go

@@ -20,7 +20,6 @@ import (
 	"github.com/juju/juju/apiserver/observer/fakeobserver"
 	apiwebsocket "github.com/juju/juju/apiserver/websocket"
 	"github.com/juju/juju/core/trace"
-	coretrace "github.com/juju/juju/core/trace"
 	"github.com/juju/juju/internal/rpcreflect"
 	"github.com/juju/juju/rpc"
 	"github.com/juju/juju/rpc/jsoncodec"
@@ -133,5 +132,5 @@ func (av allVersions) FindMethod(rootMethodName string, version int, objMethodNa
 }
 
 func (av allVersions) StartTrace(ctx context.Context) (context.Context, trace.Span) {
-	return ctx, coretrace.NoopSpan{}
+	return ctx, trace.NoopSpan{}
 }