Fix shadowed variables in various places: Part 1 of 2 (#10175)

* Fix shadowed variables in cmd package

* Fix shadowed variables in plugin package

* Fix shadowed variables in store package

* Fix shadowed variables in web package

* Changes as requested

Signed-off-by: Hanzei <hanzei@mailbox.org>

* Fix build

* Remove unnessary statements

* Use require all the time

* Fix build

* Rename variables according to feedback

* Fix NPE

* Changes as requested

cmd/mattermost/commands/sampledata.go

@@ -202,14 +202,16 @@ func sampleDataCmdF(command *cobra.Command, args []string) error {
 	}
 	profileImages := []string{}
 	if profileImagesPath != "" {
-		profileImagesStat, err := os.Stat(profileImagesPath)
+		var profileImagesStat os.FileInfo
+		profileImagesStat, err = os.Stat(profileImagesPath)
 		if os.IsNotExist(err) {
 			return errors.New("Profile images folder doesn't exists.")
 		}
 		if !profileImagesStat.IsDir() {
 			return errors.New("profile-images parameters must be a folder path.")
 		}
-		profileImagesFiles, err := ioutil.ReadDir(profileImagesPath)
+		var profileImagesFiles []os.FileInfo
+		profileImagesFiles, err = ioutil.ReadDir(profileImagesPath)
 		if err != nil {
 			return errors.New("Invalid profile-images parameter")
 		}

cmd/mattermost/commands/server_test.go

@@ -105,9 +105,9 @@ func TestRunServerSystemdNotification(t *testing.T) {
 	socketReader := make(chan string)
 	go func(ch chan string) {
 		buffer := make([]byte, 512)
-		count, err := connection.Read(buffer)
+		count, readErr := connection.Read(buffer)
-		if err != nil {
+		if readErr != nil {
-			panic(err)
+			panic(readErr)
 		}
 		data := buffer[0:count]
 		ch <- string(data)

plugin/environment.go

@@ -151,14 +151,14 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 		return nil, false, fmt.Errorf("plugin not found: %v", id)
 	}
 
-	activePlugin := activePlugin{BundleInfo: pluginInfo}
+	ap := activePlugin{BundleInfo: pluginInfo}
 	defer func() {
 		if reterr == nil {
-			activePlugin.State = model.PluginStateRunning
+			ap.State = model.PluginStateRunning
 		} else {
-			activePlugin.State = model.PluginStateFailedToStart
+			ap.State = model.PluginStateFailedToStart
 		}
-		env.activePlugins.Store(pluginInfo.Manifest.Id, activePlugin)
+		env.activePlugins.Store(pluginInfo.Manifest.Id, ap)
 	}()
 
 	if pluginInfo.Manifest.MinServerVersion != "" {
@@ -211,11 +211,11 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 	}
 
 	if pluginInfo.Manifest.HasServer() {
-		supervisor, err := newSupervisor(pluginInfo, env.logger, env.newAPIImpl(pluginInfo.Manifest))
+		sup, err := newSupervisor(pluginInfo, env.logger, env.newAPIImpl(pluginInfo.Manifest))
 		if err != nil {
 			return nil, false, errors.Wrapf(err, "unable to start plugin: %v", id)
 		}
-		activePlugin.supervisor = supervisor
+		ap.supervisor = sup
 
 		componentActivated = true
 	}
@@ -236,12 +236,12 @@ func (env *Environment) Deactivate(id string) bool {
 
 	env.activePlugins.Delete(id)
 
-	activePlugin := p.(activePlugin)
+	ap := p.(activePlugin)
-	if activePlugin.supervisor != nil {
+	if ap.supervisor != nil {
-		if err := activePlugin.supervisor.Hooks().OnDeactivate(); err != nil {
+		if err := ap.supervisor.Hooks().OnDeactivate(); err != nil {
-			env.logger.Error("Plugin OnDeactivate() error", mlog.String("plugin_id", activePlugin.BundleInfo.Manifest.Id), mlog.Err(err))
+			env.logger.Error("Plugin OnDeactivate() error", mlog.String("plugin_id", ap.BundleInfo.Manifest.Id), mlog.Err(err))
 		}
-		activePlugin.supervisor.Shutdown()
+		ap.supervisor.Shutdown()
 	}
 
 	return true
@@ -250,13 +250,13 @@ func (env *Environment) Deactivate(id string) bool {
 // Shutdown deactivates all plugins and gracefully shuts down the environment.
 func (env *Environment) Shutdown() {
 	env.activePlugins.Range(func(key, value interface{}) bool {
-		activePlugin := value.(activePlugin)
+		ap := value.(activePlugin)
 
-		if activePlugin.supervisor != nil {
+		if ap.supervisor != nil {
-			if err := activePlugin.supervisor.Hooks().OnDeactivate(); err != nil {
+			if err := ap.supervisor.Hooks().OnDeactivate(); err != nil {
-				env.logger.Error("Plugin OnDeactivate() error", mlog.String("plugin_id", activePlugin.BundleInfo.Manifest.Id), mlog.Err(err))
+				env.logger.Error("Plugin OnDeactivate() error", mlog.String("plugin_id", ap.BundleInfo.Manifest.Id), mlog.Err(err))
 			}
-			activePlugin.supervisor.Shutdown()
+			ap.supervisor.Shutdown()
 		}
 
 		env.activePlugins.Delete(key)
@@ -270,9 +270,9 @@ func (env *Environment) Shutdown() {
 // Consider using RunMultiPluginHook instead.
 func (env *Environment) HooksForPlugin(id string) (Hooks, error) {
 	if p, ok := env.activePlugins.Load(id); ok {
-		activePlugin := p.(activePlugin)
+		ap := p.(activePlugin)
-		if activePlugin.supervisor != nil {
+		if ap.supervisor != nil {
-			return activePlugin.supervisor.Hooks(), nil
+			return ap.supervisor.Hooks(), nil
 		}
 	}
 
@@ -285,12 +285,12 @@ func (env *Environment) HooksForPlugin(id string) (Hooks, error) {
 // plugins is not specified.
 func (env *Environment) RunMultiPluginHook(hookRunnerFunc func(hooks Hooks) bool, hookId int) {
 	env.activePlugins.Range(func(key, value interface{}) bool {
-		activePlugin := value.(activePlugin)
+		ap := value.(activePlugin)
 
-		if activePlugin.supervisor == nil || !activePlugin.supervisor.Implements(hookId) {
+		if ap.supervisor == nil || !ap.supervisor.Implements(hookId) {
 			return true
 		}
-		if !hookRunnerFunc(activePlugin.supervisor.Hooks()) {
+		if !hookRunnerFunc(ap.supervisor.Hooks()) {
 			return false
 		}
 

plugin/supervisor.go

@@ -11,7 +11,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/hashicorp/go-plugin"
+	plugin "github.com/hashicorp/go-plugin"
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
 )
@@ -23,10 +23,10 @@ type supervisor struct {
 }
 
 func newSupervisor(pluginInfo *model.BundleInfo, parentLogger *mlog.Logger, apiImpl API) (retSupervisor *supervisor, retErr error) {
-	supervisor := supervisor{}
+	sup := supervisor{}
 	defer func() {
 		if retErr != nil {
-			supervisor.Shutdown()
+			sup.Shutdown()
 		}
 	}()
 
@@ -53,7 +53,7 @@ func newSupervisor(pluginInfo *model.BundleInfo, parentLogger *mlog.Logger, apiI
 	}
 	executable = filepath.Join(pluginInfo.Path, executable)
 
-	supervisor.client = plugin.NewClient(&plugin.ClientConfig{
+	sup.client = plugin.NewClient(&plugin.ClientConfig{
 		HandshakeConfig: handshake,
 		Plugins:         pluginMap,
 		Cmd:             exec.Command(executable),
@@ -63,7 +63,7 @@ func newSupervisor(pluginInfo *model.BundleInfo, parentLogger *mlog.Logger, apiI
 		StartTimeout:    time.Second * 3,
 	})
 
-	rpcClient, err := supervisor.client.Client()
+	rpcClient, err := sup.client.Client()
 	if err != nil {
 		return nil, err
 	}
@@ -73,24 +73,24 @@ func newSupervisor(pluginInfo *model.BundleInfo, parentLogger *mlog.Logger, apiI
 		return nil, err
 	}
 
-	supervisor.hooks = raw.(Hooks)
+	sup.hooks = raw.(Hooks)
 
-	if impl, err := supervisor.hooks.Implemented(); err != nil {
+	impl, err := sup.hooks.Implemented()
+	if err != nil {
 		return nil, err
-	} else {
+	}
-		for _, hookName := range impl {
+	for _, hookName := range impl {
-			if hookId, ok := hookNameToId[hookName]; ok {
+		if hookId, ok := hookNameToId[hookName]; ok {
-				supervisor.implemented[hookId] = true
+			sup.implemented[hookId] = true
-			}
 		}
 	}
 
-	err = supervisor.Hooks().OnActivate()
+	err = sup.Hooks().OnActivate()
 	if err != nil {
 		return nil, err
 	}
 
-	return &supervisor, nil
+	return &sup, nil
 }
 
 func (sup *supervisor) Shutdown() {

store/storetest/channel_store.go

@@ -563,36 +563,25 @@ func testChannelStoreGetByName(t *testing.T, ss store.Store) {
 	o1.Type = model.CHANNEL_OPEN
 	store.Must(ss.Channel().Save(&o1, -1))
 
-	r1 := <-ss.Channel().GetByName(o1.TeamId, o1.Name, true)
+	result := <-ss.Channel().GetByName(o1.TeamId, o1.Name, true)
-	if r1.Err != nil {
+	require.Nil(t, result.Err)
-		t.Fatal(r1.Err)
+	require.Equal(t, o1.ToJson(), result.Data.(*model.Channel).ToJson(), "invalid returned channel")
-	} else {
-		if r1.Data.(*model.Channel).ToJson() != o1.ToJson() {
-			t.Fatal("invalid returned channel")
-		}
-	}
 
-	if err := (<-ss.Channel().GetByName(o1.TeamId, "", true)).Err; err == nil {
+	channelID := result.Data.(*model.Channel).Id
-		t.Fatal("Missing id should have failed")
-	}
 
-	if r1 := <-ss.Channel().GetByName(o1.TeamId, o1.Name, false); r1.Err != nil {
+	result = <-ss.Channel().GetByName(o1.TeamId, "", true)
-		t.Fatal(r1.Err)
+	require.NotNil(t, result.Err, "Missing id should have failed")
-	} else {
-		if r1.Data.(*model.Channel).ToJson() != o1.ToJson() {
-			t.Fatal("invalid returned channel")
-		}
-	}
 
-	if err := (<-ss.Channel().GetByName(o1.TeamId, "", false)).Err; err == nil {
+	result = <-ss.Channel().GetByName(o1.TeamId, o1.Name, false)
-		t.Fatal("Missing id should have failed")
+	require.Nil(t, result.Err)
-	}
+	require.Equal(t, o1.ToJson(), result.Data.(*model.Channel).ToJson(), "invalid returned channel")
 
-	store.Must(ss.Channel().Delete(r1.Data.(*model.Channel).Id, model.GetMillis()))
+	result = <-ss.Channel().GetByName(o1.TeamId, "", false)
+	require.NotNil(t, result.Err, "Missing id should have failed")
 
-	if err := (<-ss.Channel().GetByName(o1.TeamId, r1.Data.(*model.Channel).Name, false)).Err; err == nil {
+	store.Must(ss.Channel().Delete(channelID, model.GetMillis()))
-		t.Fatal("Deleted channel should not be returned by GetByName()")
+	result = <-ss.Channel().GetByName(o1.TeamId, o1.Name, false)
-	}
+	require.NotNil(t, result.Err, "Deleted channel should not be returned by GetByName()")
 }
 
 func testChannelStoreGetByNames(t *testing.T, ss store.Store) {

web/webhook_test.go

@@ -205,19 +205,20 @@ func TestIncomingWebhook(t *testing.T) {
 
 		hook, err := th.App.CreateIncomingWebhookForChannel(th.BasicUser.Id, th.BasicChannel, &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, ChannelLocked: true})
 		require.Nil(t, err)
+		require.NotNil(t, hook)
 
-		url := ApiClient.Url + "/hooks/" + hook.Id
+		apiHookUrl := ApiClient.Url + "/hooks/" + hook.Id
 
 		payload := "payload={\"text\": \"test text\"}"
-		resp, err2 := http.Post(url, "application/x-www-form-urlencoded", strings.NewReader(payload))
+		resp, err2 := http.Post(apiHookUrl, "application/x-www-form-urlencoded", strings.NewReader(payload))
 		require.Nil(t, err2)
 		assert.True(t, resp.StatusCode == http.StatusOK)
 
-		resp, err2 = http.Post(url, "application/json", strings.NewReader(fmt.Sprintf("{\"text\":\"this is a test\", \"channel\":\"%s\"}", th.BasicChannel.Name)))
+		resp, err2 = http.Post(apiHookUrl, "application/json", strings.NewReader(fmt.Sprintf("{\"text\":\"this is a test\", \"channel\":\"%s\"}", th.BasicChannel.Name)))
 		require.Nil(t, err2)
 		assert.True(t, resp.StatusCode == http.StatusOK)
 
-		resp, err2 = http.Post(url, "application/json", strings.NewReader(fmt.Sprintf("{\"text\":\"this is a test\", \"channel\":\"%s\"}", channel.Name)))
+		resp, err2 = http.Post(apiHookUrl, "application/json", strings.NewReader(fmt.Sprintf("{\"text\":\"this is a test\", \"channel\":\"%s\"}", channel.Name)))
 		require.Nil(t, err2)
 		assert.True(t, resp.StatusCode == http.StatusForbidden)
 	})