Add golang linters (#2192)

Enable linters for `common` and `k8sutils` modules. Rest of the modules
will be added gradually

.github/workflows/golangci-lint.yml

@@ -0,0 +1,27 @@
+name: golangci-lint
+on:
+  push:
+    branches:
+      - main
+  pull_request:
+
+permissions:
+  contents: read
+
+jobs:
+  golangci:
+    strategy:
+      matrix:
+        modules: [common, k8sutils]
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-go@v5
+        with:
+          go-version: '1.23.0'
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v6
+        with:
+          version: v1.63.4
+          working-directory: ${{ matrix.modules }}

.golangci.yml

@@ -0,0 +1,90 @@
+# TODO(edenfed): Commented out linters are good and we should enable them in the future.
+# Let's enable them one by one and fix the issues they find.
+run:
+  concurrency: 4
+  timeout: 2m
+  tests: false
+
+linters:
+  disable-all: true
+  enable:
+    - nilerr
+    - nilnesserr
+    # - nilnil
+    - bodyclose
+    - dogsled
+    - dupl
+    - noctx
+    - errcheck
+    - funlen
+    - goconst
+    - gocritic
+    - gocyclo
+    - gofmt
+    - goimports
+#    - revive
+    - gosec
+    - gosimple
+    - govet
+    - ineffassign
+    - lll
+    - misspell
+    - nakedret
+    - staticcheck
+    # - stylecheck
+    - typecheck
+    - unconvert
+    - unparam
+    - whitespace
+    - unused
+
+linters-settings:
+  dupl:
+    threshold: 200
+  goconst:
+    min-len: 6
+    min-occurrences: 3
+  misspell:
+    locale: US
+  lll:
+    line-length: 150
+  goimports:
+    local-prefixes: github.com/odigos-io
+  gocritic:
+    settings:
+      hugeParam:
+        sizeThreshold: 100
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - opinionated
+      - performance
+      - style
+    disabled-checks:
+      - ifElseChain
+      - unnamedResult
+      - paramTypeCombine
+  funlen:
+    lines: 110
+    statements: 60
+  whitespace:
+    multi-if: false
+    multi-func: false
+  gosec:
+    includes:
+      - G401
+      - G306
+      - G110
+      - G111
+      - G114
+      - G112
+    excludes:
+      - G204
+      - G101
+
+issues:
+  exclude-rules:
+    - path: _test\.go
+      linters:
+        - unparam
+        - funlen

Makefile

@@ -1,6 +1,43 @@
 TAG ?= $(shell odigos version --cluster)
 ODIGOS_CLI_VERSION ?= $(shell odigos version --cli)
 ORG ?= keyval
+GOLANGCI_LINT_VERSION ?= v1.63.4
+GOLANGCI_LINT := $(shell go env GOPATH)/bin/golangci-lint
+GO_MODULES := $(shell find . -type f -name "go.mod" -not -path "*/vendor/*" -exec dirname {} \;)
+LINT_CMD = golangci-lint run -c ../.golangci.yml
+ifdef FIX_LINT
+    LINT_CMD += --fix
+endif
+
+.PHONY: install-golangci-lint
+install-golangci-lint:
+	@if ! which golangci-lint >/dev/null || [ "$$(golangci-lint version 2>&1 | head -n 1 | awk '{print "v"$$4}')" != "$(GOLANGCI_LINT_VERSION)" ]; then \
+		echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION)..."; \
+		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $$(go env GOPATH)/bin $(GOLANGCI_LINT_VERSION); \
+	else \
+		echo "golangci-lint $(GOLANGCI_LINT_VERSION) is already installed"; \
+	fi
+
+.PHONY: lint
+lint: install-golangci-lint
+ifdef MODULE
+	@echo "Running lint for module: $(MODULE)"
+	@if [ ! -d "$(MODULE)" ]; then \
+		echo "Error: Directory $(MODULE) does not exist"; \
+		exit 1; \
+	fi
+	@if [ ! -f "$(MODULE)/go.mod" ]; then \
+		echo "Error: $(MODULE) is not a Go module (no go.mod found)"; \
+		exit 1; \
+	fi
+	@cd $(MODULE) && $(LINT_CMD) ./...
+else
+	@echo "No MODULE specified, running lint for all Go modules..."
+	@for module in $(GO_MODULES); do \
+		echo "Running lint for $$module"; \
+		(cd $$module && $(LINT_CMD) ./...) || exit 1; \
+	done
+endif
 
 .PHONY: build-odiglet
 build-odiglet:

cli/cmd/diagnose_util/crd_utils.go

@@ -3,16 +3,17 @@ package diagnose_util
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+
 	"github.com/odigos-io/odigos/cli/cmd/resources"
 	"github.com/odigos-io/odigos/cli/pkg/kube"
 	"github.com/odigos-io/odigos/k8sutils/pkg/client"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"os"
-	"path/filepath"
 	"sigs.k8s.io/yaml"
-	"sync"
 )
 
 const (
@@ -105,7 +106,7 @@ func fetchSingleResource(ctx context.Context, kubeClient *kube.Client, crdDataDi
 		Resource: resourceData[CRDName],  // The resourceData type
 	}
 
-	err := client.ListWithPages(client.DefaultPageSize, kubeClient.Dynamic.Resource(gvr).List, ctx, metav1.ListOptions{}, func(crds *unstructured.UnstructuredList) error {
+	err := client.ListWithPages(client.DefaultPageSize, kubeClient.Dynamic.Resource(gvr).List, ctx, &metav1.ListOptions{}, func(crds *unstructured.UnstructuredList) error {
 		for _, crd := range crds.Items {
 			if err := saveCrdToFile(crd, crdDataDirPath, crd.GetName()); err != nil {
 				fmt.Printf("Fetching Resource %s Failed because: %s\n", resourceData[CRDName], err)

common/config/axiom.go

@@ -18,7 +18,6 @@ func (a *Axiom) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) err
 	dataset, exists := dest.GetConfig()[axiomDatasetKey]
 	if !exists {
 		dataset = "default"
-		// ctrl.Log.V(0).Info("Axiom dataset not specified, using default")
 	}
 
 	axiomExporterName := "otlphttp/axiom-" + dest.GetID()

common/config/causely.go

@@ -3,9 +3,10 @@ package config
 import (
 	"errors"
 	"fmt"
-	"github.com/odigos-io/odigos/common"
 	"net/url"
 	"strings"
+
+	"github.com/odigos-io/odigos/common"
 )
 
 const (

common/config/chronosphere.go

@@ -23,7 +23,6 @@ func (c *Chronosphere) DestType() common.DestinationType {
 }
 
 func (c *Chronosphere) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	url, exists := dest.GetConfig()[chronosphereDomain]
 	if !exists {
 		return ErrorChronosphereMissingURL
@@ -87,7 +86,7 @@ func (c *Chronosphere) getCompanyNameFromURL(url string) string {
 	// Remove trailing slash if present
 	url = strings.TrimSuffix(url, "/")
 
-	// Support the following cases: COMAPNY / COMPANY.chronosphere.io / COMPANY.chronosphere.io:443
+	// Support the following cases: COMPANY / COMPANY.chronosphere.io / COMPANY.chronosphere.io:443
 	url = strings.TrimSuffix(url, ".chronosphere.io:443")
 	url = strings.TrimSuffix(url, ".chronosphere.io")
 	return url

common/config/dynatrace.go

@@ -25,7 +25,6 @@ func (n *Dynatrace) DestType() common.DestinationType {
 }
 
 func (n *Dynatrace) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	if !n.requiredVarsExists(dest) {
 		return errors.New("Dynatrace config is missing required variables")
 	}

common/config/elasticapm.go

@@ -19,17 +19,14 @@ func (e *ElasticAPM) DestType() common.DestinationType {
 }
 
 func (e *ElasticAPM) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-	var isTlsDisabled = false
+	isTlsDisabled := false
 	if !e.requiredVarsExists(dest) {
 		return errors.New("ElasticAPM config is missing required variables")
 	}
 
 	isTlsDisabled = strings.Contains(dest.GetConfig()[elasticApmServerEndpoint], "http://")
 
-	elasticApmEndpoint, err := e.parseEndpoint(dest.GetConfig()[elasticApmServerEndpoint])
-	if err != nil {
-		return errors.Join(err, errors.New("ElasticAPM endpoint is not a valid"))
-	}
+	elasticApmEndpoint := e.parseEndpoint(dest.GetConfig()[elasticApmServerEndpoint])
 
 	exporterName := "otlp/elastic-" + dest.GetID()
 	currentConfig.Exporters[exporterName] = GenericMap{
@@ -74,14 +71,14 @@ func (e *ElasticAPM) requiredVarsExists(dest ExporterConfigurer) bool {
 	return true
 }
 
-func (e *ElasticAPM) parseEndpoint(endpoint string) (string, error) {
+func (e *ElasticAPM) parseEndpoint(endpoint string) string {
 	var port = "8200"
-	endpoint = strings.Trim(endpoint, "http://")
-	endpoint = strings.Trim(endpoint, "https://")
+	endpoint = strings.TrimPrefix(endpoint, "http://")
+	endpoint = strings.TrimPrefix(endpoint, "https://")
 	endpointDetails := strings.Split(endpoint, ":")
 	host := endpointDetails[0]
 	if len(endpointDetails) > 1 {
 		port = endpointDetails[1]
 	}
-	return fmt.Sprintf("%s:%s", host, port), nil
+	return fmt.Sprintf("%s:%s", host, port)
 }

common/config/elasticsearch.go

@@ -12,11 +12,13 @@ const (
 	ElasticsearchUrlKey = "ELASTICSEARCH_URL"
 	esTracesIndexKey    = "ES_TRACES_INDEX"
 	esLogsIndexKey      = "ES_LOGS_INDEX"
-	esBasicAuthKey      = "ELASTICSEARCH_BASIC_AUTH_ENABLED" // unused in this file, currently UI only (we do not want to break existing setups by requiring this boolean)
-	esUsername          = "ELASTICSEARCH_USERNAME"
-	esPassword          = "ELASTICSEARCH_PASSWORD"
-	esTlsKey            = "ELASTICSEARCH_TLS_ENABLED" // unused in this file, currently UI only (we do not want to break existing setups by requiring this boolean)
-	esCaPem             = "ELASTICSEARCH_CA_PEM"
+	esBasicAuthKey      = "ELASTICSEARCH_BASIC_AUTH_ENABLED" // unused in this file,
+	// currently UI only (we do not want to break existing setups by requiring this boolean)
+	esUsername = "ELASTICSEARCH_USERNAME"
+	esPassword = "ELASTICSEARCH_PASSWORD"
+	esTlsKey   = "ELASTICSEARCH_TLS_ENABLED" // unused in this file,
+	// currently UI only (we do not want to break existing setups by requiring this boolean)
+	esCaPem = "ELASTICSEARCH_CA_PEM"
 )
 
 var _ Configer = (*Elasticsearch)(nil)
@@ -35,7 +37,7 @@ func (e *Elasticsearch) ModifyConfig(dest ExporterConfigurer, currentConfig *Con
 
 	parsedURL, err := e.SanitizeURL(rawURL)
 	if err != nil {
-		return errors.Join(err, errors.New(fmt.Sprintf("failed to sanitize URL. elasticsearch-url: %s", rawURL)))
+		return errors.Join(err, fmt.Errorf("failed to sanitize URL. elasticsearch-url: %s", rawURL))
 	}
 
 	traceIndexVal, exists := dest.GetConfig()[esTracesIndexKey]
@@ -89,8 +91,8 @@ func (e *Elasticsearch) ModifyConfig(dest ExporterConfigurer, currentConfig *Con
 
 // SanitizeURL will check whether URL is correct by utilizing url.ParseRequestURI
 // if the said URL has not defined any port, 9200 will be used in order to keep the backward compatibility with current configuration
-func (e *Elasticsearch) SanitizeURL(URL string) (string, error) {
-	parsedURL, err := url.ParseRequestURI(URL)
+func (e *Elasticsearch) SanitizeURL(link string) (string, error) {
+	parsedURL, err := url.ParseRequestURI(link)
 	if err != nil {
 		return "", err
 	}

common/config/gcp.go

@@ -11,7 +11,6 @@ func (g *GoogleCloud) DestType() common.DestinationType {
 }
 
 func (g *GoogleCloud) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	if isTracingEnabled(dest) {
 		exporterName := "googlecloud/" + dest.GetID()
 		currentConfig.Exporters[exporterName] = struct{}{}

common/config/gcs.go

@@ -18,14 +18,12 @@ func (g *GoogleCloudStorage) DestType() common.DestinationType {
 }
 
 func (g *GoogleCloudStorage) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	if !isTracingEnabled(dest) && !isLoggingEnabled(dest) {
 		return errors.New("GoogleCloudStorage is not enabled for any supported signals, skipping")
 	}
 
 	bucket, ok := dest.GetConfig()[gcsBucketKey]
 	if !ok {
-		// log.Log.V(0).Info("GCS bucket not specified, using default bucket %s", defaultGCSBucket)
 		bucket = defaultGCSBucket
 	}
 

common/config/genericotlp.go

@@ -17,7 +17,6 @@ func (g *GenericOTLP) DestType() common.DestinationType {
 }
 
 func (g *GenericOTLP) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	url, exists := dest.GetConfig()[genericOtlpUrlKey]
 	if !exists {
 		return errors.New("Generic OTLP gRPC endpoint not specified, gateway will not be configured for otlp")

common/config/grafanacloudloki.go

@@ -22,7 +22,6 @@ func (g *GrafanaCloudLoki) DestType() common.DestinationType {
 }
 
 func (g *GrafanaCloudLoki) ModifyConfig(dest ExporterConfigurer, currentConfig *Config) error {
-
 	if !isLoggingEnabled(dest) {
 		return errors.New("Logging not enabled, gateway will not be configured for grafana cloud Loki")
 	}
@@ -94,7 +93,6 @@ func (g *GrafanaCloudLoki) ModifyConfig(dest ExporterConfigurer, currentConfig *
 // this function will attempt to parse and prepare the url for use with the
 // otelcol loki exporter
 func grafanaLokiUrlFromInput(rawUrl string) (string, error) {
-
 	rawUrl = strings.TrimSpace(rawUrl)
 	urlWithScheme := rawUrl
 
@@ -113,9 +111,9 @@ func grafanaLokiUrlFromInput(rawUrl string) (string, error) {
 	}
 
 	if parsedUrl.Path == "" {
-		parsedUrl.Path = "/loki/api/v1/push"
+		parsedUrl.Path = lokiApiPath
 	}
-	if parsedUrl.Path != "/loki/api/v1/push" {
+	if parsedUrl.Path != lokiApiPath {
 		return "", fmt.Errorf("unexpected path for loki endpoint %s", parsedUrl.Path)
 	}