Skip to content

Commit

Permalink
Linter fixes
Browse files Browse the repository at this point in the history
Golangci-linter configuration ".ci-operator.yaml" was configured in a way
which was showing a lot of false positives and also missing a lot of issues.

Fix both the linter configuration and the missing issues that found its way
into the codebase.
  • Loading branch information
jmencak committed Dec 12, 2024
1 parent 729cd59 commit 2fc45c3
Show file tree
Hide file tree
Showing 74 changed files with 429 additions and 359 deletions.
2 changes: 1 addition & 1 deletion .ci-operator.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
build_root_image:
name: release
namespace: openshift
tag: rhel-9-release-golang-1.22-openshift-4.18
tag: rhel-9-release-golang-1.23-openshift-4.19
21 changes: 15 additions & 6 deletions .golangci.yaml
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
run:
timeout: 5m
tests: false
skip-dirs:
- vendor
- test
modules-download-mode: vendor
issues:
exclude-dirs:
- vendor
exclude-rules:
# Temporarily disable the deprecation warnings until we rewrite the code based on deprecated functionality.
- linters:
- staticcheck
text: "SA1019: .* deprecated"
linters:
disable-all: true
enable:
- errcheck
- bodyclose
- exportloopref
- gosimple
- govet
- ineffassign
Expand All @@ -25,8 +28,14 @@ linters:
- wastedassign
- whitespace
linters-settings:
errcheck:
# report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`;
# default is false: such cases aren't reported by default.
check-blank: false
misspell:
locale: US
# Do not set locale explicitly, the default is to use a neutral variety of English.
# Setting locale to US will cause correcting the British spelling of 'colour' to 'color'.
# locale: US
ignore-words:
- NTO
- nto
4 changes: 2 additions & 2 deletions Dockerfile.rhel9
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.22-openshift-4.18 AS builder
FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.23-openshift-4.19 AS builder
WORKDIR /go/src/github.com/openshift/cluster-node-tuning-operator
COPY . .
RUN make build

FROM registry.ci.openshift.org/ocp/4.18:base-rhel9
FROM registry.ci.openshift.org/ocp/4.19:base-rhel9
COPY --from=builder /go/src/github.com/openshift/cluster-node-tuning-operator/_output/cluster-node-tuning-operator /usr/bin/
COPY --from=builder /go/src/github.com/openshift/cluster-node-tuning-operator/_output/performance-profile-creator /usr/bin/
COPY --from=builder /go/src/github.com/openshift/cluster-node-tuning-operator/_output/gather-sysinfo /usr/bin/
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ PAO_CRD_APIS :=$(addprefix ./$(API_TYPES_DIR)/performanceprofile/,v2 v1 v1alpha1
PAO_E2E_SUITES := $(shell hack/list-test-bin.sh)

# golangci-lint variables
GOLANGCI_LINT_VERSION=1.54.2
GOLANGCI_LINT_VERSION=1.62.2
GOLANGCI_LINT_BIN=$(OUT_DIR)/golangci-lint
GOLANGCI_LINT_VERSION_TAG=v${GOLANGCI_LINT_VERSION}

Expand Down
3 changes: 1 addition & 2 deletions cmd/gather-sysinfo/gather-sysinfo_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package main

import (
"io/ioutil"
"os"
"strings"
"testing"
Expand Down Expand Up @@ -63,7 +62,7 @@ func TestCollectMachineInfo(t *testing.T) {
t.Errorf("Collection of machine info failed: %v", err)
}

content, err := ioutil.ReadFile(destFile)
content, err := os.ReadFile(destFile)
if err != nil {
t.Errorf("Reading of generated output failed: %v", err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/performanceprofile/performance_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package performance

import (
"io/ioutil"
"os"
"strings"

"github.com/RHsyseng/operator-utils/pkg/validation"
Expand Down Expand Up @@ -53,7 +53,7 @@ var _ = Describe("PerformanceProfile CR(D) Schema", func() {
// getSchema reads in & returns CRD schema file as openAPIV3Schema{} for validation usage.
// See references operator-utils/validation/schema & go-openapi/spec/schema
func getSchema(crdPath string) (validation.Schema, error) {
bytes, err := ioutil.ReadFile(crdPath)
bytes, err := os.ReadFile(crdPath)
if err != nil {
return nil, err
}
Expand All @@ -66,7 +66,7 @@ func getSchema(crdPath string) (validation.Schema, error) {

// getCR unmarshals a *_cr.yaml file and returns the representing struct
func getCR(crPath string) (map[string]interface{}, error) {
bytes, err := ioutil.ReadFile(crPath)
bytes, err := os.ReadFile(crPath)
if err != nil {
return nil, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,11 @@ func validateNoIntersectionExists(lists *components.CPULists, allErrs field.Erro
func (r *PerformanceProfile) validateSelectors() field.ErrorList {
var allErrs field.ErrorList

if r.Spec.MachineConfigLabel != nil && len(r.Spec.MachineConfigLabel) > 1 {
if len(r.Spec.MachineConfigLabel) > 1 {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec.machineConfigLabel"), r.Spec.MachineConfigLabel, "you should provide only 1 MachineConfigLabel"))
}

if r.Spec.MachineConfigPoolSelector != nil && len(r.Spec.MachineConfigPoolSelector) > 1 {
if len(r.Spec.MachineConfigPoolSelector) > 1 {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec.machineConfigPoolSelector"), r.Spec.MachineConfigLabel, "you should provide only 1 MachineConfigPoolSelector"))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
"k8s.io/utils/ptr"
)

const (
Expand Down Expand Up @@ -126,13 +126,13 @@ func NewPerformanceProfile(name string) *PerformanceProfile {
},
},
RealTimeKernel: &RealTimeKernel{
Enabled: pointer.Bool(true),
Enabled: ptr.To(true),
},
NUMA: &NUMA{
TopologyPolicy: &numaPolicy,
},
Net: &Net{
UserLevelNetworking: pointer.Bool(true),
UserLevelNetworking: ptr.To(true),
Devices: []Device{
{
InterfaceName: &netDeviceName,
Expand Down Expand Up @@ -381,7 +381,7 @@ var _ = Describe("PerformanceProfile", func() {
setValidNodeSelector(profile)

errors = profile.validateSelectors()
Expect(profile.validateSelectors()).To(BeEmpty(), "should not have validation errors when machine config selector nil")
Expect(errors).To(BeEmpty(), "should not have validation errors when machine config selector nil")
})

It("should should have 0 or 1 MachineConfigPoolSelector labels", func() {
Expand All @@ -397,7 +397,7 @@ var _ = Describe("PerformanceProfile", func() {
setValidNodeSelector(profile)

errors = profile.validateSelectors()
Expect(profile.validateSelectors()).To(BeEmpty(), "should not have validation errors when machine config pool selector nil")
Expect(errors).To(BeEmpty(), "should not have validation errors when machine config pool selector nil")
})

It("should have sensible NodeSelector in case MachineConfigLabel or MachineConfigPoolSelector is empty", func() {
Expand Down Expand Up @@ -583,7 +583,7 @@ var _ = Describe("PerformanceProfile", func() {

profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 128,
Node: pointer.Int32(0),
Node: ptr.To(int32(0)),
Size: "14M",
})
errors := profile.validateHugePages(nodes)
Expand All @@ -604,7 +604,7 @@ var _ = Describe("PerformanceProfile", func() {

profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 128,
Node: pointer.Int32(0),
Node: ptr.To(int32(0)),
Size: "14M",
})
errors := profile.validateHugePages(nodes)
Expand Down Expand Up @@ -635,7 +635,7 @@ var _ = Describe("PerformanceProfile", func() {

profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 128,
Node: pointer.Int32(0),
Node: ptr.To(int32(0)),
Size: "14M",
})

Expand All @@ -661,12 +661,12 @@ var _ = Describe("PerformanceProfile", func() {
profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 128,
Size: hugepagesSize1G,
Node: pointer.Int32(0),
Node: ptr.To(int32(0)),
})
profile.Spec.HugePages.Pages = append(profile.Spec.HugePages.Pages, HugePage{
Count: 64,
Size: hugepagesSize1G,
Node: pointer.Int32(0),
Node: ptr.To(int32(0)),
})
errors := profile.validateHugePages(nodes)
Expect(errors).NotTo(BeEmpty())
Expand Down Expand Up @@ -729,33 +729,33 @@ var _ = Describe("PerformanceProfile", func() {
It("should raise the validation syntax errors", func() {
invalidVendor := "123"
invalidDevice := "0x12345"
profile.Spec.Net.Devices[0].InterfaceName = pointer.String("")
profile.Spec.Net.Devices[0].VendorID = pointer.String(invalidVendor)
profile.Spec.Net.Devices[0].DeviceID = pointer.String(invalidDevice)
profile.Spec.Net.Devices[0].InterfaceName = ptr.To("")
profile.Spec.Net.Devices[0].VendorID = ptr.To(invalidVendor)
profile.Spec.Net.Devices[0].DeviceID = ptr.To(invalidDevice)
errors := profile.validateNet()
Expect(len(errors)).To(Equal(3))
Expect(errors[0].Error()).To(ContainSubstring(fmt.Sprintf("device name cannot be empty")))
Expect(errors[0].Error()).To(ContainSubstring("device name cannot be empty"))
Expect(errors[1].Error()).To(ContainSubstring(fmt.Sprintf("device vendor ID %s has an invalid format. Vendor ID should be represented as 0x<4 hexadecimal digits> (16 bit representation)", invalidVendor)))
Expect(errors[2].Error()).To(ContainSubstring(fmt.Sprintf("device model ID %s has an invalid format. Model ID should be represented as 0x<4 hexadecimal digits> (16 bit representation)", invalidDevice)))

})
It("should raise the validation errors for missing fields", func() {
profile.Spec.Net.Devices[0].VendorID = nil
profile.Spec.Net.Devices[0].DeviceID = pointer.String("0x1")
profile.Spec.Net.Devices[0].DeviceID = ptr.To("0x1")
errors := profile.validateNet()
Expect(errors).NotTo(BeEmpty())
Expect(errors[0].Error()).To(ContainSubstring(fmt.Sprintf("device model ID can not be used without specifying the device vendor ID.")))
Expect(errors[0].Error()).To(ContainSubstring("device model ID can not be used without specifying the device vendor ID."))
})
})

Describe("Workload hints validation", func() {
When("realtime kernel is enabled and realtime workload hint is explicitly disabled", func() {
It("should raise validation error", func() {
profile.Spec.WorkloadHints = &WorkloadHints{
RealTime: pointer.Bool(false),
RealTime: ptr.To(false),
}
profile.Spec.RealTimeKernel = &RealTimeKernel{
Enabled: pointer.Bool(true),
Enabled: ptr.To(true),
}
errors := profile.validateWorkloadHints()
Expect(errors).NotTo(BeEmpty())
Expand All @@ -765,8 +765,8 @@ var _ = Describe("PerformanceProfile", func() {
When("HighPowerConsumption hint is enabled and PerPodPowerManagement hint is enabled", func() {
It("should raise validation error", func() {
profile.Spec.WorkloadHints = &WorkloadHints{
HighPowerConsumption: pointer.Bool(true),
PerPodPowerManagement: pointer.Bool(true),
HighPowerConsumption: ptr.To(true),
PerPodPowerManagement: ptr.To(true),
}
errors := profile.validateWorkloadHints()
Expect(errors).NotTo(BeEmpty())
Expand All @@ -776,7 +776,7 @@ var _ = Describe("PerformanceProfile", func() {
When("MixedCPUs hint is enabled but no shared CPUs are specified", func() {
It("should raise validation error", func() {
profile.Spec.WorkloadHints = &WorkloadHints{
MixedCpus: pointer.Bool(true),
MixedCpus: ptr.To(true),
}
errors := profile.validateWorkloadHints()
Expect(errors).NotTo(BeEmpty())
Expand Down Expand Up @@ -807,17 +807,17 @@ var _ = Describe("PerformanceProfile", func() {
profile.Spec.HugePages.DefaultHugePagesSize = &incorrectDefaultSize

profile.Spec.WorkloadHints = &WorkloadHints{
RealTime: pointer.Bool(false),
RealTime: ptr.To(false),
}
profile.Spec.RealTimeKernel = &RealTimeKernel{
Enabled: pointer.Bool(true),
Enabled: ptr.To(true),
}

invalidVendor := "123"
invalidDevice := "0x12345"
profile.Spec.Net.Devices[0].InterfaceName = pointer.String("")
profile.Spec.Net.Devices[0].VendorID = pointer.String(invalidVendor)
profile.Spec.Net.Devices[0].DeviceID = pointer.String(invalidDevice)
profile.Spec.Net.Devices[0].InterfaceName = ptr.To("")
profile.Spec.Net.Devices[0].VendorID = ptr.To(invalidVendor)
profile.Spec.Net.Devices[0].DeviceID = ptr.To(invalidDevice)

errors := profile.ValidateBasicFields()

Expand Down
13 changes: 7 additions & 6 deletions pkg/operator/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package operator

import (
"context"
"errors"
"fmt"
"os"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
operatorv1helpers "github.com/openshift/library-go/pkg/operator/v1helpers"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -80,7 +81,7 @@ func (c *Controller) syncOperatorStatus(tuned *tunedv1.Tuned) error {
func (c *Controller) getOrCreateOperatorStatus() (*configv1.ClusterOperator, error) {
co, err := c.listers.ClusterOperators.Get(tunedv1.TunedClusterOperatorResourceName)
if err != nil {
if errors.IsNotFound(err) {
if apierrors.IsNotFound(err) {
// Cluster operator not found, create it
co = &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: tunedv1.TunedClusterOperatorResourceName}}
co, err = c.clients.ConfigV1Client.ClusterOperators().Create(context.TODO(), co, metav1.CreateOptions{})
Expand Down Expand Up @@ -205,7 +206,7 @@ func (c *Controller) computeStatus(tuned *tunedv1.Tuned, conditions []configv1.C
ds, err := c.listers.DaemonSets.Get(dsMf.Name)
if err != nil {
// There was a problem fetching Tuned daemonset
if errors.IsNotFound(err) {
if apierrors.IsNotFound(err) {
// Tuned daemonset has not been created yet
if len(conditions) == 0 {
// This looks like a fresh install => initialize
Expand Down Expand Up @@ -238,7 +239,7 @@ func (c *Controller) computeStatus(tuned *tunedv1.Tuned, conditions []configv1.C
} else {
if ds.Status.ObservedGeneration != ds.Generation {
// Do not base the conditions on stale information
return conditions, "", fmt.Errorf(errGenerationMismatch)
return conditions, "", errors.New(errGenerationMismatch)
}

dsReleaseVersion := c.getDaemonSetReleaseVersion(ds)
Expand Down Expand Up @@ -303,14 +304,14 @@ func (c *Controller) computeStatus(tuned *tunedv1.Tuned, conditions []configv1.C
}

if numDegradedProfiles > 0 {
klog.Infof(fmt.Sprintf("%v/%v Profiles failed to be applied", numDegradedProfiles, len(profileList)))
klog.Infof("%v/%v Profiles failed to be applied", numDegradedProfiles, len(profileList))
availableCondition.Reason = "ProfileDegraded"
availableCondition.Message = fmt.Sprintf("%v/%v Profiles failed to be applied", numDegradedProfiles, len(profileList))
}

numConflict := c.numProfilesWithBootcmdlineConflict(profileList)
if numConflict > 0 {
klog.Infof(fmt.Sprintf("%v/%v Profiles with bootcmdline conflict", numConflict, len(profileList)))
klog.Infof("%v/%v Profiles with bootcmdline conflict", numConflict, len(profileList))
degradedCondition.Status = configv1.ConditionTrue
degradedCondition.Reason = "ProfileConflict"
degradedCondition.Message = fmt.Sprintf("%v/%v Profiles with bootcmdline conflict", numConflict, len(profileList))
Expand Down
Loading

0 comments on commit 2fc45c3

Please sign in to comment.