Fix loop pointer references (#1982)

Miguel Varela Ramos · vishalbollu · vishalbollu · commit 1696fe71d05b · 2021-03-22T14:32:35.000Z
* Fix saving pointer reference in for/range loop * Fix or ignore looppointer warnings * Add looppointer to lint.sh and tools * Fix looppointer ignores * Update circleci config.yml * Propagate non-zero exit code * Remove nolint:looppointer labels Co-authored-by: vishal <vishalbollu@users.noreply.github.com> Co-authored-by: Vishal Bollu <vishal.bollu@gmail.com> (cherry picked from commit 971a091)
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -98,8 +98,12 @@ jobs:
       - checkout
       - setup_remote_docker
       - install-go
-      - run: go get -u -v golang.org/x/lint/golint
-      - run: sudo pip install black aiohttp
+      - run:
+         name: Install Linting Tools
+         command: |
+           go get -u -v golang.org/x/lint/golint
+           go get -u -v github.com/kyoh86/looppointer/cmd/looppointer
+           sudo pip install black aiohttp
       - run:
           name: Lint
           command: make lint
diff --git a/Makefile b/Makefile
@@ -225,6 +225,7 @@ registry-clean-aws:
 
 tools:
 	@go get -u -v golang.org/x/lint/golint
+	@go get -u -v github.com/kyoh86/looppointer/cmd/looppointer
 	@go get -u -v github.com/VojtechVitek/rerun/cmd/rerun
 	@go get -u -v github.com/go-delve/delve/cmd/dlv
 	@if [[ "$$OSTYPE" == "darwin"* ]]; then brew install parallel; elif [[ "$$OSTYPE" == "linux"* ]]; then sudo apt-get install -y parallel; else echo "your operating system is not supported"; fi
diff --git a/build/lint.sh b/build/lint.sh
@@ -34,6 +34,11 @@ if ! command -v golint >/dev/null 2>&1; then
   exit 1
 fi
 
+if ! command -v looppointer >/dev/null 2>&1; then
+  echo "looppointer must be installed"
+  exit 1
+fi
+
 if ! command -v gofmt >/dev/null 2>&1; then
   echo "gofmt must be installed"
   exit 1
@@ -52,6 +57,12 @@ if [[ $output ]]; then
   exit 1
 fi
 
+output=$(looppointer "$ROOT/...")
+if [[ $output ]]; then
+  echo "$output"
+  exit 1
+fi
+
 output=$(gofmt -s -l "$ROOT")
 if [[ $output ]]; then
   echo "go files not properly formatted:"
diff --git a/pkg/lib/archive/archiver.go b/pkg/lib/archive/archiver.go
@@ -46,29 +46,29 @@ func archive(input *Input, arc archiver) (strset.Set, error) {
 	addedPaths := strset.New()
 	var err error
 
-	for _, byteInput := range input.Bytes {
-		err = addBytesToArchive(&byteInput, input, arc, addedPaths)
+	for i := range input.Bytes {
+		err = addBytesToArchive(&input.Bytes[i], input, arc, addedPaths)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	for _, fileInput := range input.Files {
-		err = addFileToArchive(&fileInput, input, arc, addedPaths)
+	for i := range input.Files {
+		err = addFileToArchive(&input.Files[i], input, arc, addedPaths)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	for _, dirInput := range input.Dirs {
-		err = addDirToArchive(&dirInput, input, arc, addedPaths)
+	for i := range input.Dirs {
+		err = addDirToArchive(&input.Dirs[i], input, arc, addedPaths)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	for _, fileListInput := range input.FileLists {
-		err = addFileListToArchive(&fileListInput, input, arc, addedPaths)
+	for i := range input.FileLists {
+		err = addFileListToArchive(&input.FileLists[i], input, arc, addedPaths)
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/lib/k8s/pod.go b/pkg/lib/k8s/pod.go
@@ -134,7 +134,9 @@ func IsPodReady(pod *kcore.Pod) bool {
 }
 
 func GetPodReadyTime(pod *kcore.Pod) *time.Time {
-	for _, condition := range pod.Status.Conditions {
+	for i := range pod.Status.Conditions {
+		condition := pod.Status.Conditions[i]
+
 		if condition.Type == "Ready" && condition.Status == kcore.ConditionTrue {
 			if condition.LastTransitionTime.Time.IsZero() {
 				return nil
diff --git a/pkg/operator/endpoints/info.go b/pkg/operator/endpoints/info.go
@@ -93,7 +93,9 @@ func getNodeInfos() ([]schema.NodeInfo, int, error) {
 	nodeInfoMap := make(map[string]*schema.NodeInfo, len(nodes)) // node name -> info
 	spotPriceCache := make(map[string]float64)                   // instance type -> spot price
 
-	for _, node := range nodes {
+	for i := range nodes {
+		node := nodes[i]
+
 		instanceType := node.Labels["beta.kubernetes.io/instance-type"]
 		nodeGroupName := node.Labels["alpha.eksctl.io/nodegroup-name"]
 		isSpot := strings.Contains(strings.ToLower(node.Labels["lifecycle"]), "spot")
@@ -128,7 +130,9 @@ func getNodeInfos() ([]schema.NodeInfo, int, error) {
 
 	var numPendingReplicas int
 
-	for _, pod := range pods {
+	for i := range pods {
+		pod := pods[i]
+
 		_, isAPIPod := pod.Labels["apiName"]
 
 		if pod.Spec.NodeName == "" && isAPIPod {
diff --git a/pkg/operator/main.go b/pkg/operator/main.go
@@ -71,7 +71,8 @@ func main() {
 			exit.Error(errors.Wrap(err, "init"))
 		}
 
-		for _, deployment := range deployments {
+		for i := range deployments {
+			deployment := deployments[i]
 			apiKind := deployment.Labels["apiKind"]
 			if userconfig.KindFromString(apiKind) == userconfig.RealtimeAPIKind ||
 				userconfig.KindFromString(apiKind) == userconfig.AsyncAPIKind {
diff --git a/pkg/operator/resources/asyncapi/api.go b/pkg/operator/resources/asyncapi/api.go
@@ -223,7 +223,8 @@ func GetAllAPIs(pods []kcore.Pod, deployments []kapps.Deployment) ([]schema.APIR
 
 	realtimeAPIs := make([]schema.APIResponse, len(apis))
 
-	for i, api := range apis {
+	for i := range apis {
+		api := apis[i]
 		endpoint, err := operator.APIEndpoint(&api)
 		if err != nil {
 			return nil, err
diff --git a/pkg/operator/resources/asyncapi/status.go b/pkg/operator/resources/asyncapi/status.go
@@ -237,7 +237,9 @@ func getReplicaCounts(deployment *kapps.Deployment, pods []kcore.Pod) status.Rep
 	counts := status.ReplicaCounts{}
 	counts.Requested = *deployment.Spec.Replicas
 
-	for _, pod := range pods {
+	for i := range pods {
+		pod := pods[i]
+
 		if pod.Labels["apiName"] != deployment.Labels["apiName"] {
 			continue
 		}
diff --git a/pkg/operator/resources/job/batchapi/api.go b/pkg/operator/resources/job/batchapi/api.go
@@ -128,8 +128,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
 	batchAPIsMap := map[string]*schema.APIResponse{}
 
 	jobIDToK8sJobMap := map[string]*kbatch.Job{}
-	for _, kJob := range k8sJobs {
-		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob
+	for i, kJob := range k8sJobs {
+		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
 	}
 
 	jobIDToPodsMap := map[string][]kcore.Pod{}
@@ -229,8 +229,8 @@ func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResp
 	}
 
 	jobIDToK8sJobMap := map[string]*kbatch.Job{}
-	for _, kJob := range k8sJobs {
-		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob
+	for i, kJob := range k8sJobs {
+		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
 	}
 
 	endpoint, err := operator.APIEndpoint(api)
diff --git a/pkg/operator/resources/job/batchapi/cron.go b/pkg/operator/resources/job/batchapi/cron.go
@@ -364,7 +364,8 @@ func checkForJobFailure(jobKey spec.JobKey, k8sJob kbatch.Job) (bool, error) {
 
 	reasonFound := false
 	pods, _ := config.K8s.ListPodsByLabel("jobID", jobKey.ID)
-	for _, pod := range pods {
+	for i := range pods {
+		pod := pods[i] // to avoid loop pointer bugs
 		if k8s.WasPodOOMKilled(&pod) {
 			jobLogger.Error("at least one worker was killed because it ran out of out of memory")
 			return true, errors.FirstError(
diff --git a/pkg/operator/resources/job/taskapi/api.go b/pkg/operator/resources/job/taskapi/api.go
@@ -123,8 +123,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
 	taskAPIsMap := map[string]*schema.APIResponse{}
 
 	jobIDToK8sJobMap := map[string]*kbatch.Job{}
-	for _, job := range k8sJobs {
-		jobIDToK8sJobMap[job.Labels["jobID"]] = &job
+	for i, kJob := range k8sJobs {
+		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
 	}
 
 	jobIDToPodsMap := map[string][]kcore.Pod{}
@@ -204,7 +204,7 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
 	return taskAPIList, nil
 }
 
-// GetAllAPIs returns a single task API and its most recently submitted job along with all running task jobs
+// GetAPIByName returns a single task API and its most recently submitted job along with all running task jobs
 func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResponse, error) {
 	virtualService := deployedResource.VirtualService
 
@@ -220,8 +220,8 @@ func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResp
 	}
 
 	jobIDToK8sJobMap := map[string]*kbatch.Job{}
-	for _, job := range k8sJobs {
-		jobIDToK8sJobMap[job.Labels["jobID"]] = &job
+	for i, kJob := range k8sJobs {
+		jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
 	}
 
 	endpoint, err := operator.APIEndpoint(api)
diff --git a/pkg/operator/resources/job/taskapi/cron.go b/pkg/operator/resources/job/taskapi/cron.go
@@ -203,8 +203,8 @@ func reconcileInProgressJob(jobState *job.State, jobFound bool) (status.JobCode,
 
 func checkIfJobCompleted(jobKey spec.JobKey, k8sJob kbatch.Job) error {
 	pods, _ := config.K8s.ListPodsByLabel("jobID", jobKey.ID)
-	for _, pod := range pods {
-		if k8s.WasPodOOMKilled(&pod) {
+	for i := range pods {
+		if k8s.WasPodOOMKilled(&pods[i]) {
 			return errors.FirstError(
 				job.SetWorkerOOMStatus(jobKey),
 				deleteJobRuntimeResources(jobKey),
diff --git a/pkg/operator/resources/job/worker_stats.go b/pkg/operator/resources/job/worker_stats.go
@@ -35,8 +35,8 @@ func GetWorkerCountsForJob(k8sJob kbatch.Job, pods []kcore.Pod) status.WorkerCou
 	}
 
 	workerCounts := status.WorkerCounts{}
-	for _, pod := range pods {
-		addPodToWorkerCounts(&pod, &workerCounts)
+	for i := range pods {
+		addPodToWorkerCounts(&pods[i], &workerCounts)
 	}
 
 	return workerCounts
diff --git a/pkg/operator/resources/realtimeapi/api.go b/pkg/operator/resources/realtimeapi/api.go
@@ -200,7 +200,8 @@ func GetAllAPIs(pods []kcore.Pod, deployments []kapps.Deployment) ([]schema.APIR
 
 	realtimeAPIs := make([]schema.APIResponse, len(apis))
 
-	for i, api := range apis {
+	for i := range apis {
+		api := apis[i]
 		endpoint, err := operator.APIEndpoint(&api)
 		if err != nil {
 			return nil, err
diff --git a/pkg/operator/resources/realtimeapi/status.go b/pkg/operator/resources/realtimeapi/status.go
@@ -62,12 +62,12 @@ func GetStatus(apiName string) (*status.Status, error) {
 
 func GetAllStatuses(deployments []kapps.Deployment, pods []kcore.Pod) ([]status.Status, error) {
 	statuses := make([]status.Status, len(deployments))
-	for i, deployment := range deployments {
-		status, err := apiStatus(&deployment, pods)
+	for i := range deployments {
+		st, err := apiStatus(&deployments[i], pods)
 		if err != nil {
 			return nil, err
 		}
-		statuses[i] = *status
+		statuses[i] = *st
 	}
 
 	sort.Slice(statuses, func(i, j int) bool {
@@ -96,11 +96,12 @@ func getReplicaCounts(deployment *kapps.Deployment, pods []kcore.Pod) status.Rep
 	counts := status.ReplicaCounts{}
 	counts.Requested = *deployment.Spec.Replicas
 
-	for _, pod := range pods {
+	for i := range pods {
+		pod := pods[i]
 		if pod.Labels["apiName"] != deployment.Labels["apiName"] {
 			continue
 		}
-		addPodToReplicaCounts(&pod, deployment, &counts)
+		addPodToReplicaCounts(&pods[i], deployment, &counts)
 	}
 
 	return counts
diff --git a/pkg/operator/resources/trafficsplitter/api.go b/pkg/operator/resources/trafficsplitter/api.go
@@ -138,7 +138,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService) ([]schem
 		return nil, err
 	}
 
-	for _, trafficSplitter := range apis {
+	for i := range apis {
+		trafficSplitter := apis[i]
 		endpoint, err := operator.APIEndpoint(&trafficSplitter)
 		if err != nil {
 			return nil, err
diff --git a/pkg/operator/resources/validations.go b/pkg/operator/resources/validations.go
@@ -235,7 +235,8 @@ func awsManagedValidateK8sCompute(compute *userconfig.Compute, maxMemMap map[str
 }
 
 func validateEndpointCollisions(api *userconfig.API, virtualServices []istioclientnetworking.VirtualService) error {
-	for _, virtualService := range virtualServices {
+	for i := range virtualServices {
+		virtualService := virtualServices[i]
 		gateways := k8s.ExtractVirtualServiceGateways(&virtualService)
 		if !gateways.Has("apis-gateway") {
 			continue

Original file line number	Diff line number	Diff line change
`@@ -46,29 +46,29 @@ func archive(input *Input, arc archiver) (strset.Set, error) {`
`46`	`46`	`addedPaths := strset.New()`
`47`	`47`	`var err error`
`48`	`48`
`49`		`- for _, byteInput := range input.Bytes {`
`50`		`- err = addBytesToArchive(&byteInput, input, arc, addedPaths)`
	`49`	`+ for i := range input.Bytes {`
	`50`	`+ err = addBytesToArchive(&input.Bytes[i], input, arc, addedPaths)`
`51`	`51`	`if err != nil {`
`52`	`52`	`return nil, err`
`53`	`53`	`}`
`54`	`54`	`}`
`55`	`55`
`56`		`- for _, fileInput := range input.Files {`
`57`		`- err = addFileToArchive(&fileInput, input, arc, addedPaths)`
	`56`	`+ for i := range input.Files {`
	`57`	`+ err = addFileToArchive(&input.Files[i], input, arc, addedPaths)`
`58`	`58`	`if err != nil {`
`59`	`59`	`return nil, err`
`60`	`60`	`}`
`61`	`61`	`}`
`62`	`62`
`63`		`- for _, dirInput := range input.Dirs {`
`64`		`- err = addDirToArchive(&dirInput, input, arc, addedPaths)`
	`63`	`+ for i := range input.Dirs {`
	`64`	`+ err = addDirToArchive(&input.Dirs[i], input, arc, addedPaths)`
`65`	`65`	`if err != nil {`
`66`	`66`	`return nil, err`
`67`	`67`	`}`
`68`	`68`	`}`
`69`	`69`
`70`		`- for _, fileListInput := range input.FileLists {`
`71`		`- err = addFileListToArchive(&fileListInput, input, arc, addedPaths)`
	`70`	`+ for i := range input.FileLists {`
	`71`	`+ err = addFileListToArchive(&input.FileLists[i], input, arc, addedPaths)`
`72`	`72`	`if err != nil {`
`73`	`73`	`return nil, err`
`74`	`74`	`}`
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,8 @@ func main() {`
`71`	`71`	`exit.Error(errors.Wrap(err, "init"))`
`72`	`72`	`}`
`73`	`73`
`74`		`- for _, deployment := range deployments {`
	`74`	`+ for i := range deployments {`
	`75`	`+ deployment := deployments[i]`
`75`	`76`	`apiKind := deployment.Labels["apiKind"]`
`76`	`77`	`if userconfig.KindFromString(apiKind) == userconfig.RealtimeAPIKind \|\|`
`77`	`78`	`userconfig.KindFromString(apiKind) == userconfig.AsyncAPIKind {`
Original file line number	Diff line number	Diff line change
`@@ -128,8 +128,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs`
`128`	`128`	`batchAPIsMap := map[string]*schema.APIResponse{}`
`129`	`129`
`130`	`130`	`jobIDToK8sJobMap := map[string]*kbatch.Job{}`
`131`		`- for _, kJob := range k8sJobs {`
`132`		`- jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob`
	`131`	`+ for i, kJob := range k8sJobs {`
	`132`	`+ jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`jobIDToPodsMap := map[string][]kcore.Pod{}`
`@@ -229,8 +229,8 @@ func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResp`
`229`	`229`	`}`
`230`	`230`
`231`	`231`	`jobIDToK8sJobMap := map[string]*kbatch.Job{}`
`232`		`- for _, kJob := range k8sJobs {`
`233`		`- jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob`
	`232`	`+ for i, kJob := range k8sJobs {`
	`233`	`+ jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]`
`234`	`234`	`}`
`235`	`235`
`236`	`236`	`endpoint, err := operator.APIEndpoint(api)`