Ineffassign fixes

The following are potential juju issues spotted by ineffassign linter. We should really consider cherry-picking these as they're potentially real world fixes. Before doing that we should really also check if every one is valid.
orbital-transfer-survey · Aug 15, 2019 · 8fb1073 · 8fb1073
1 parent 599ee1b
commit 8fb1073
Show file tree

Hide file tree

Showing 55 changed files with 123 additions and 46 deletions.
diff --git a/apiserver/facades/agent/storageprovisioner/shim.go b/apiserver/facades/agent/storageprovisioner/shim.go
@@ -33,7 +33,11 @@ func NewFacadeV3(st *state.State, resources facade.Resources, authorizer facade.
 	registry, err := stateenvirons.NewStorageProviderRegistryForModel(
 		model,
 		stateenvirons.GetNewEnvironFunc(environs.New),
-		stateenvirons.GetNewCAASBrokerFunc(caas.New))
+		stateenvirons.GetNewCAASBrokerFunc(caas.New),
+	)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
 	pm := poolmanager.New(state.NewStateSettings(st), registry)
 
 	backend, storageBackend, err := NewStateBackends(st)

diff --git a/apiserver/facades/agent/uniter/uniter.go b/apiserver/facades/agent/uniter/uniter.go
@@ -1219,6 +1219,9 @@ func (u *UniterAPI) RelationsStatus(args params.Entities) (params.RelationUnitSt
 			return nil, errors.Trace(err)
 		}
 		relations, err := app.Relations()
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
 		for _, rel := range relations {
 			rus, err := oneRelationUnitStatus(rel, unit)
 			if err != nil {

diff --git a/apiserver/facades/client/application/charmstore.go b/apiserver/facades/client/application/charmstore.go
@@ -117,6 +117,9 @@ func AddCharmWithAuthorizationAndRepo(st State, args params.AddCharmWithAuthoriz
 
 	// Get the repo from the constructor
 	repo, err := repoFn()
+	if err != nil {
+		return errors.Trace(err)
+	}
 
 	// Get the charm and its information from the store.
 	downloadedCharm, err := repo.Get(charmURL)

diff --git a/apiserver/facades/client/applicationoffers/base.go b/apiserver/facades/client/applicationoffers/base.go
@@ -118,7 +118,7 @@ func (api *BaseAPI) applicationOffersFromModel(
 
 	// If requireAdmin is true, the user must be a controller superuser
 	// or model admin to proceed.
-	isAdmin := false
+	var isAdmin bool
 	err = api.checkAdmin(backend)
 	if err != nil && err != common.ErrPerm {
 		return nil, errors.Trace(err)

diff --git a/apiserver/facades/client/client/status.go b/apiserver/facades/client/client/status.go
@@ -133,12 +133,9 @@ func (c *Client) StatusHistory(request params.StatusHistoryRequests) params.Stat
 			continue
 		}
 
-		var (
-			err  error
-			hist []params.DetailedStatus
-		)
+		var err error
+		var hist []params.DetailedStatus
 		kind := status.HistoryKind(request.Kind)
-		err = errors.NotValidf("%q requires a unit, got %T", kind, request.Tag)
 		switch kind {
 		case status.KindUnit, status.KindWorkload, status.KindUnitAgent:
 			var u names.UnitTag
@@ -662,6 +659,9 @@ func fetchAllApplicationsAndUnits(
 	unitMap := make(map[string]map[string]*state.Unit)
 	latestCharms := make(map[charm.URL]*state.Charm)
 	applications, err := st.AllApplications()
+	if err != nil {
+		return applicationStatusInfo{}, err
+	}
 	units, err := model.AllUnits()
 	if err != nil {
 		return applicationStatusInfo{}, err

diff --git a/apiserver/facades/client/keymanager/keymanager.go b/apiserver/facades/client/keymanager/keymanager.go
@@ -262,6 +262,7 @@ func runSSHKeyImport(keyIds []string) map[string][]importedSSHKey {
 		output, err := RunSSHImportId(keyId)
 		if err != nil {
 			keyInfo = append(keyInfo, importedSSHKey{err: err})
+			importResults[keyId] = keyInfo
 			continue
 		}
 		lines := strings.Split(output, "\n")

diff --git a/apiserver/facades/client/machinemanager/instance_information.go b/apiserver/facades/client/machinemanager/instance_information.go
@@ -42,6 +42,9 @@ func instanceTypes(mm *MachineManagerAPI,
 	}
 
 	env, err := getEnviron(backend, environs.New)
+	if err != nil {
+		return params.InstanceTypesResults{}, errors.Trace(err)
+	}
 	result := make([]params.InstanceTypesResult, len(cons.Constraints))
 	// TODO(perrito666) Cache the results to avoid excessive querying of the cloud.
 	for i, c := range cons.Constraints {

diff --git a/apiserver/gui.go b/apiserver/gui.go
@@ -590,6 +590,9 @@ func (h *guiArchiveHandler) handlePost(w http.ResponseWriter, req *http.Request)
 
 	// Read and validate the archive data.
 	data, hash, err := readAndHash(req.Body)
+	if err != nil {
+		return errors.Trace(err)
+	}
 	size := int64(len(data))
 	if size != req.ContentLength {
 		return errors.BadRequestf("archive does not match provided content length")

diff --git a/apiserver/stateauthenticator/modeluser.go b/apiserver/stateauthenticator/modeluser.go
@@ -166,7 +166,8 @@ func (u *modelUserEntity) UpdateLastLogin() error {
 			return errors.NotValidf("%s as model user", u.modelUser.Object.Kind())
 		}
 
-		model, err := u.st.Model()
+		var model *state.Model
+		model, err = u.st.Model()
 		if err != nil {
 			return errors.Trace(err)
 		}
@@ -177,7 +178,7 @@ func (u *modelUserEntity) UpdateLastLogin() error {
 	if u.user != nil {
 		err1 := u.user.UpdateLastLogin()
 		if err == nil {
-			return err1
+			return errors.Trace(err1)
 		}
 	}
 	if err != nil {

diff --git a/caas/kubernetes/provider/k8s.go b/caas/kubernetes/provider/k8s.go
@@ -2285,6 +2285,9 @@ func (k *kubernetesClient) Operator(appName string) (*caas.Operator, error) {
 	terminated := opPod.DeletionTimestamp != nil
 	now := time.Now()
 	statusMessage, opStatus, since, err := k.getPODStatus(opPod, now)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
 	return &caas.Operator{
 		Id:    string(opPod.UID),
 		Dying: terminated,

diff --git a/charmstore/fakeclient.go b/charmstore/fakeclient.go
@@ -28,6 +28,7 @@ import (
 	"crypto/sha512"
 	"fmt"
 	"io"
+	"reflect"
 
 	"github.com/juju/errors"
 	"github.com/juju/utils/set"
@@ -56,7 +57,11 @@ func (d datastore) Get(path string, data interface{}) error {
 	if current == nil {
 		return errors.NotFoundf(path)
 	}
-	data = current
+	value := reflect.ValueOf(data)
+	if value.Type().Kind() != reflect.Ptr {
+		return errors.New("data not a ptr")
+	}
+	value.Elem().Set(reflect.ValueOf(current))
 	return nil
 }
 

diff --git a/cmd/juju/backups/backups.go b/cmd/juju/backups/backups.go
@@ -95,7 +95,7 @@ var getAPI = func(c *CommandBase) (APIClient, int, error) {
 	}
 	version := root.BestFacadeVersion("Backups")
 	client, err := backups.NewClient(root)
-	return client, version, nil
+	return client, version, errors.Trace(err)
 }
 
 // dumpMetadata writes the formatted backup metadata to stdout.

diff --git a/cmd/juju/commands/bootstrap_clouds.go b/cmd/juju/commands/bootstrap_clouds.go
@@ -63,7 +63,6 @@ func formatCloudDetailsTabular(ctx *cmd.Context, clouds cloudList, credStore juj
 				} else {
 					p("", credName, "")
 				}
-				i++
 			}
 		}
 		return nil

diff --git a/cmd/juju/common/authkeys.go b/cmd/juju/common/authkeys.go
@@ -44,7 +44,7 @@ func FinalizeAuthorizedKeys(ctx *cmd.Context, attrs map[string]interface{}) erro
 	}
 	coercedAttrs := coerced.(map[string]interface{})
 
-	authorizedKeys, haveAuthorizedKeys := coercedAttrs[config.AuthorizedKeysKey].(string)
+	_, haveAuthorizedKeys := coercedAttrs[config.AuthorizedKeysKey].(string)
 	authorizedKeysPath, haveAuthorizedKeysPath := coercedAttrs[authorizedKeysPathKey].(string)
 	if haveAuthorizedKeys && haveAuthorizedKeysPath {
 		return errors.Errorf(
@@ -57,7 +57,7 @@ func FinalizeAuthorizedKeys(ctx *cmd.Context, attrs map[string]interface{}) erro
 		return nil
 	}
 
-	authorizedKeys, err = ReadAuthorizedKeys(ctx, authorizedKeysPath)
+	authorizedKeys, err := ReadAuthorizedKeys(ctx, authorizedKeysPath)
 	if err != nil {
 		return errors.Annotate(err, "reading authorized-keys")
 	}

diff --git a/cmd/juju/controller/listmodels.go b/cmd/juju/controller/listmodels.go
@@ -117,7 +117,7 @@ func (c *modelsCommand) Run(ctx *cmd.Context) error {
 	}
 	defer modelmanagerAPI.Close()
 
-	haveModels := false
+	var haveModels bool
 	if modelmanagerAPI.BestAPIVersion() > 3 {
 		haveModels, err = c.getModelSummaries(ctx, modelmanagerAPI, now)
 		if err != nil {

diff --git a/cmd/juju/resource/deploy.go b/cmd/juju/resource/deploy.go
@@ -140,7 +140,8 @@ func (d deployUploader) validateResourceDetails(res map[string]string) error {
 		case charmresource.TypeFile:
 			err = d.checkFile(name, value)
 		case charmresource.TypeContainerImage:
-			dockerDetails, err := getDockerDetailsData(value, d.osOpen)
+			var dockerDetails resources.DockerImageDetails
+			dockerDetails, err = getDockerDetailsData(value, d.osOpen)
 			if err != nil {
 				return err
 			}

diff --git a/cmd/juju/user/login.go b/cmd/juju/user/login.go
@@ -576,7 +576,7 @@ func (c *loginCommand) promptUserToTrustCA(ctx *cmd.Context, ctrlDetails *jujucl
 			prettyName = fmt.Sprintf("%q (%s)", host, endpoint)
 		}
 		fmt.Fprintf(ctx.Stderr, "Controller %s presented a CA cert that could not be verified.\nCA fingerprint: [%s]\n", prettyName, fingerprint)
-		trust, err := c.pollster.YN("Trust remote controller", false)
+		trust, _ := c.pollster.YN("Trust remote controller", false)
 		if !trust {
 			return errors.New("controller CA not trusted")
 		}

diff --git a/cmd/jujud/agent/agenttest/agent.go b/cmd/jujud/agent/agenttest/agent.go
@@ -138,6 +138,7 @@ func (s *AgentSuite) PrimeAgentVersion(c *gc.C, tag names.Tag, password string,
 	c.Assert(err, jc.ErrorIsNil)
 	agentTools := envtesting.PrimeTools(c, stor, s.DataDir(), "released", vers)
 	err = envtools.MergeAndWriteMetadata(stor, "released", "released", coretools.List{agentTools}, envtools.DoNotWriteMirrors)
+	c.Assert(err, jc.ErrorIsNil)
 	tools1, err := agenttools.ChangeAgentTools(s.DataDir(), tag.String(), vers)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(tools1, gc.DeepEquals, agentTools)

diff --git a/cmd/jujud/agent/machine.go b/cmd/jujud/agent/machine.go
@@ -948,7 +948,7 @@ func (a *MachineAgent) initController(agentConfig agent.Config) (*state.Controll
 		NewPolicy:              stateenvirons.GetNewPolicyFunc(),
 		RunTransactionObserver: a.mongoTxnCollector.AfterRunTransaction,
 	})
-	return ctrl, nil
+	return ctrl, errors.Trace(err)
 }
 
 func (a *MachineAgent) initState(agentConfig agent.Config) (*state.StatePool, error) {

diff --git a/cmd/jujud/main.go b/cmd/jujud/main.go
@@ -231,7 +231,7 @@ func Main(args []string) int {
 		os.Exit(exit_err)
 	}
 
-	code := 1
+	var code int
 	commandName := filepath.Base(args[0])
 	switch commandName {
 	case names.Jujud:

diff --git a/cmd/plugins/juju-metadata/signmetadata.go b/cmd/plugins/juju-metadata/signmetadata.go
@@ -82,6 +82,9 @@ func process(dir, key, passphrase string) error {
 	logger.Debugf("processing directory %q", dir)
 	// Do any json files in dir
 	filenames, err := filepath.Glob(filepath.Join(dir, "*"+simplestreams.UnsignedSuffix))
+	if err != nil {
+		return err
+	}
 	if len(filenames) > 0 {
 		logger.Infof("signing %d file(s) in %q", len(filenames), dir)
 	}

diff --git a/container/lxd/image.go b/container/lxd/image.go
@@ -48,6 +48,9 @@ func (s *Server) FindImage(
 	localAlias := seriesLocalAlias(series, arch)
 	var target string
 	entry, _, err := s.GetImageAlias(localAlias)
+	if err != nil {
+		return SourcedImage{}, errors.Trace(err)
+	}
 	if entry != nil {
 		// We already have an image with the given alias, so just use that.
 		target = entry.Target

diff --git a/container/lxd/instance.go b/container/lxd/instance.go
@@ -40,14 +40,14 @@ func (lxd *lxdInstance) Addresses(ctx context.ProviderCallContext) ([]corenetwor
 
 // Status implements instances.Instance.Status.
 func (lxd *lxdInstance) Status(ctx context.ProviderCallContext) instance.Status {
-	jujuStatus := status.Pending
 	instStatus, _, err := lxd.server.GetContainerState(lxd.id)
 	if err != nil {
 		return instance.Status{
 			Status:  status.Empty,
 			Message: fmt.Sprintf("could not get status: %v", err),
 		}
 	}
+	var jujuStatus status.Status
 	switch instStatus.StatusCode {
 	case api.Starting, api.Started:
 		jujuStatus = status.Allocating

diff --git a/core/model/upgradeseries.go b/core/model/upgradeseries.go
@@ -46,8 +46,10 @@ func ValidateUpgradeSeriesStatus(status UpgradeSeriesStatus) (UpgradeSeriesStatu
 // the second is greater -1 is returned; 1 is returned otherwise. An error is
 // returned if either argument is an invalid status.
 func CompareUpgradeSeriesStatus(status1 UpgradeSeriesStatus, status2 UpgradeSeriesStatus) (int, error) {
-	var err error
 	st1, err := ValidateUpgradeSeriesStatus(status1)
+	if err != nil {
+		return 0, err
+	}
 	st2, err := ValidateUpgradeSeriesStatus(status2)
 	if err != nil {
 		return 0, err

diff --git a/doc.go b/doc.go
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-// Juju is devops distilled.
+// Package juju is devops distilled.
 //
 // Project homepage: https://github.com/juju/juju
 //

diff --git a/environs/jujutest/livetests.go b/environs/jujutest/livetests.go
@@ -454,6 +454,7 @@ func (t *LiveTests) TestPorts(c *gc.C) {
 		})
 	c.Assert(err, jc.ErrorIsNil)
 	rules, err = fwInst1.IngressRules(t.ProviderCallContext, "1")
+	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(rules, gc.HasLen, 0)
 
 	// Check that we can close ports that aren't there.
@@ -465,6 +466,7 @@ func (t *LiveTests) TestPorts(c *gc.C) {
 		})
 	c.Assert(err, jc.ErrorIsNil)
 	rules, err = fwInst2.IngressRules(t.ProviderCallContext, "2")
+	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(
 		rules, jc.DeepEquals,
 		[]network.IngressRule{

diff --git a/network/iptables/iptables.go b/network/iptables/iptables.go
@@ -249,7 +249,7 @@ func parseIngressRule(line string) (network.IngressRule, bool, error) {
 			// "tcp dpt:N" or "udp dpt:N".
 			return fail(errors.New("unexpected parameter prefix"))
 		}
-		field, line, ok = popField(line)
+		field, _, ok = popField(line)
 		if !ok || !strings.HasPrefix(field, "dpt:") {
 			return fail(errors.New("could not extract destination port"))
 		}

diff --git a/network/scriptrunner.go b/network/scriptrunner.go
@@ -49,5 +49,5 @@ func runCommand(command string, environ []string, clock clock.Clock, timeout tim
 		Stderr:   result.Stderr,
 		Code:     result.Code,
 		TimedOut: timedOut,
-	}, nil
+	}, errors.Trace(err)
 }
diff --git a/provider/azure/environ.go b/provider/azure/environ.go
@@ -1672,7 +1672,7 @@ func (env *azureEnviron) deleteControllerManagedResourceGroups(ctx context.Provi
 
 	// Walk all the pages of results so we can get a total list of groups to remove.
 	var groupNames []*string
-	for ; result.NotDone(); err = result.NextWithContext(sdkCtx) {
+	for ; result.NotDone(); result.NextWithContext(sdkCtx) {
 		for _, group := range result.Values() {
 			groupNames = append(groupNames, group.Name)
 		}

diff --git a/provider/azure/instance.go b/provider/azure/instance.go
@@ -41,7 +41,7 @@ func (inst *azureInstance) Id() instance.Id {
 
 // Status is specified in the Instance interface.
 func (inst *azureInstance) Status(ctx context.ProviderCallContext) instance.Status {
-	instanceStatus := status.Empty
+	var instanceStatus status.Status
 	message := inst.provisioningState
 	switch inst.provisioningState {
 	case "Succeeded":
@@ -114,7 +114,7 @@ func instanceNetworkInterfaces(
 		return nil, nil
 	}
 	instanceNics := make(map[instance.Id][]network.Interface)
-	for ; nicsResult.NotDone(); err = nicsResult.NextWithContext(sdkCtx) {
+	for ; nicsResult.NotDone(); nicsResult.NextWithContext(sdkCtx) {
 		nic := nicsResult.Value()
 		instanceId := instance.Id(to.String(nic.Tags[jujuMachineNameTag]))
 		instanceNics[instanceId] = append(instanceNics[instanceId], nic)
@@ -139,7 +139,7 @@ func instancePublicIPAddresses(
 		return nil, nil
 	}
 	instancePips := make(map[instance.Id][]network.PublicIPAddress)
-	for ; pipsResult.NotDone(); err = pipsResult.NextWithContext(sdkCtx) {
+	for ; pipsResult.NotDone(); pipsResult.NextWithContext(sdkCtx) {
 		pip := pipsResult.Value()
 		instanceId := instance.Id(to.String(pip.Tags[jujuMachineNameTag]))
 		instancePips[instanceId] = append(instancePips[instanceId], pip)

diff --git a/provider/cloudsigma/instance.go b/provider/cloudsigma/instance.go
@@ -34,7 +34,7 @@ func (i sigmaInstance) Id() instance.Id {
 func (i sigmaInstance) Status(ctx context.ProviderCallContext) instance.Status {
 	entityStatus := i.server.Status()
 	logger.Tracef("sigmaInstance.Status: %s", entityStatus)
-	jujuStatus := status.Pending
+	var jujuStatus status.Status
 	switch entityStatus {
 	case gosigma.ServerStarting:
 		jujuStatus = status.Allocating

diff --git a/provider/ec2/instance.go b/provider/ec2/instance.go
@@ -35,7 +35,7 @@ func (inst *ec2Instance) Id() instance.Id {
 
 func (inst *ec2Instance) Status(ctx context.ProviderCallContext) instance.Status {
 	// pending | running | shutting-down | terminated | stopping | stopped
-	jujuStatus := status.Pending
+	var jujuStatus status.Status
 	switch inst.State.Name {
 	case "pending":
 		jujuStatus = status.Pending

diff --git a/provider/gce/instance.go b/provider/gce/instance.go
@@ -37,7 +37,7 @@ func (inst *environInstance) Id() instance.Id {
 // Status implements instances.Instance.
 func (inst *environInstance) Status(ctx context.ProviderCallContext) instance.Status {
 	instStatus := inst.base.Status()
-	jujuStatus := status.Provisioning
+	var jujuStatus status.Status
 	switch instStatus {
 	case "PROVISIONING", "STAGING":
 		jujuStatus = status.Provisioning

diff --git a/provider/joyent/instance.go b/provider/joyent/instance.go
@@ -26,7 +26,7 @@ func (inst *joyentInstance) Id() instance.Id {
 
 func (inst *joyentInstance) Status(ctx context.ProviderCallContext) instance.Status {
 	instStatus := inst.machine.State
-	jujuStatus := status.Pending
+	var jujuStatus status.Status
 	switch instStatus {
 	case "configured", "incomplete", "unavailable", "provisioning":
 		jujuStatus = status.Allocating
-Original file line number
+Diff line change
@@ Expand Up @@
     				} else {
     					p("", credName, "")
     				}
-    				i++
     			}
     		}
     		return nil
@@ Expand Down @@