openshift: Add make verify and test

Also fix lint issues hightlighted by these tests.

Author: mdbooth

openshift/.golangci.yml

@@ -0,0 +1,187 @@
+linters:
+  disable-all: true
+  enable:
+  - asasalint
+  - asciicheck
+  - bidichk
+  - bodyclose
+  - cyclop
+#  - depguard
+  - dogsled
+  - dupword
+  - durationcheck
+  - errcheck
+  - exportloopref
+  - forbidigo
+  - gci
+  - goconst
+  - gocritic
+  - gocyclo
+  - godot
+  - gofmt
+  - gofumpt
+  - goheader
+# - goimports Conflicts with gci
+  - gomodguard
+  - goprintffuncname
+  - gosec
+  - gosimple
+  - govet
+  - importas
+  - ineffassign
+  - makezero
+  - misspell
+  - nakedret
+  - nestif
+  - nilerr
+  - noctx
+  - nolintlint
+  - prealloc
+  - predeclared
+  - revive
+  - rowserrcheck
+  - sqlclosecheck
+  - staticcheck
+  - stylecheck
+  - thelper
+  - typecheck
+  - unconvert
+  - unparam
+  - unused
+  - wastedassign
+  - whitespace
+
+linters-settings:
+  cyclop:
+    # TODO(sbuerin) fix remaining findings and set to 20 afterwards
+    max-complexity: 30
+  gci:
+    sections:
+    - standard
+    - default
+    - prefix(sigs.k8s.io/cluster-api-provider-openstack)
+    - prefix(github.com/openshift/cluster-api-provider-openstack/openshift)
+    skip-vendor: true
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - performance
+    disabled-checks:
+      - appendAssign
+      - dupImport # https://github.com/go-critic/go-critic/issues/845
+      - evalOrder
+      - ifElseChain
+      - octalLiteral
+      - regexpSimplify
+      - sloppyReassign
+      - truncateCmp
+      - typeDefFirst
+      - unnamedResult
+      - unnecessaryDefer
+      - whyNoLint
+      - wrapperFunc
+      - rangeValCopy
+      - hugeParam
+  importas:
+    no-unaliased: true
+    alias:
+      # Kubernetes
+      - pkg: k8s.io/api/core/v1
+        alias: corev1
+      - pkg: k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1
+        alias: apiextensionsv1
+      - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
+        alias: metav1
+      - pkg: k8s.io/apimachinery/pkg/api/errors
+        alias: apierrors
+      - pkg: k8s.io/apimachinery/pkg/util/errors
+        alias: kerrors
+      # Controller Runtime
+      - pkg: sigs.k8s.io/controller-runtime
+        alias: ctrl
+      # CAPO
+      - pkg: sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha5
+        alias: infrav1alpha5
+      - pkg: sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha6
+        alias: infrav1alpha6
+      - pkg: sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7
+        alias: infrav1
+      - pkg: sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors
+        alias: capoerrors
+      # CAPI
+      - pkg: sigs.k8s.io/cluster-api/api/v1alpha3
+        alias: clusterv1alpha3
+      - pkg: sigs.k8s.io/cluster-api/api/v1alpha4
+        alias: clusterv1alpha4
+      - pkg: sigs.k8s.io/cluster-api/api/v1beta1
+        alias: clusterv1
+      # CABPK
+      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha3
+        alias: bootstrapv1alpha3
+      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1alpha4
+        alias: bootstrapv1alpha4
+      - pkg: sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1
+        alias: bootstrapv1
+      # KCP
+      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3
+        alias: controlplanev1alpha3
+      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha4
+        alias: controlplanev1alpha4
+      - pkg: sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1
+        alias: controlplanev1
+
+  nolintlint:
+    # https://github.com/golangci/golangci-lint/issues/3228
+    allow-unused: true
+  staticcheck:
+    go: "1.17"
+  stylecheck:
+    go: "1.17"
+  nestif:
+    # minimal complexity of if statements to report, 5 by default
+    # TODO(sbuerin) fix remaining findings and set to 5 after:
+    # https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/778
+    min-complexity: 13
+
+issues:
+  max-same-issues: 0
+  max-issues-per-linter: 0
+  # List of regexps of issue texts to exclude, empty list by default.
+  exclude-rules:
+  - linters:
+    - gosec
+    text: "G108: Profiling endpoint is automatically exposed on /debug/pprof"
+  - linters:
+    - gosec
+    text: "G108: Profiling endpoint is automatically exposed on /debug/pprof"
+  # This directive allows the embed package to be imported with an underscore everywhere.
+  - linters:
+    - revive
+    source: _ "embed"
+  - linters:
+    - revive
+    - stylecheck
+    path: (test)/.*.go
+    text: should not use dot imports
+  - linters:
+    - revive
+    path: test/e2e/shared/defaults.go
+    text: "exported: exported const .* should have comment \\(or a comment on this block\\) or be unexported"
+  - linters:
+    - revive
+    text: "var-naming: don't use underscores in Go names;"
+    path: .*(api|types)\/.*\/.*conversion.*\.go$
+  - linters:
+    - stylecheck
+    text: "ST1003: should not use underscores in Go names;"
+    path: .*(api|types)\/.*\/.*conversion.*\.go$
+
+run:
+  timeout: 10m
+  build-tags:
+  - e2e
+
+  skip-files:
+    - "zz_generated.*\\.go$"
+  allow-parallel-runners: true

openshift/Makefile

@@ -18,6 +18,7 @@ manifests_prefix ?= 0000_30_cluster-api-provider-openstack_
 TOOLS_DIR=../hack/tools
 KUSTOMIZE=$(TOOLS_DIR)/bin/kustomize
 CONTROLLER_GEN=$(TOOLS_DIR)/bin/controller-gen
+GOLANGCI_LINT=$(TOOLS_DIR)/bin/golangci-lint
 
 define manifest_name
     $(addsuffix ".yaml",$(addprefix $(manifests_dir)/$(manifests_prefix),$(1)))
@@ -27,8 +28,8 @@ manifest_names = 00_credentials-request 04_infrastructure-components
 infrastructure_components = kustomize/cluster-capi-configmap/infrastructure-components.yaml
 infracluster_role = kustomize/infracluster-controller/role.yaml
 
-.PHONY: all_manifests
-all_manifests: $(foreach m,$(manifest_names),$(call manifest_name,$(m)))
+.PHONY: generate
+generate: $(foreach m,$(manifest_names),$(call manifest_name,$(m)))
 
 $(call manifest_name,00_credentials-request): $(KUSTOMIZE) ALWAYS | $(manifests_dir)
 	$(KUSTOMIZE) build kustomize/credentials-request > $@
@@ -40,7 +41,7 @@ $(call manifest_name,04_infrastructure-components): $(KUSTOMIZE) $(infrastructur
 	$(KUSTOMIZE) build kustomize/cluster-capi-configmap > $@
 
 $(infracluster_role): $(CONTROLLER_GEN) ALWAYS
-	$(CONTROLLER_GEN) rbac:roleName=infracluster-controller paths=./pkg/infracluster_controller output:stdout > $@
+	$(CONTROLLER_GEN) rbac:roleName=infracluster-controller paths=./pkg/infraclustercontroller output:stdout > $@
 
 $(manifests_dir):
 	mkdir -p $@
@@ -51,5 +52,32 @@ $(KUSTOMIZE):
 $(CONTROLLER_GEN):
 	$(MAKE) -C $(TOOLS_DIR) bin/controller-gen
 
+$(GOLANGCI_LINT):
+	$(MAKE) -C $(TOOLS_DIR) bin/golangci-lint
+
+.PHONY: verify
+verify: lint modules generate
+	@if !(git diff --quiet HEAD); then \
+		git diff; \
+		echo "generated files are out of date, run make generate"; exit 1; \
+	fi
+
+.PHONY: lint
+lint: $(GOLANGCI_LINT) ## Lint codebase
+	$(GOLANGCI_LINT) run -v --fast=false
+
+.PHONY: lint-update
+lint-update: $(GOLANGCI_LINT) ## Lint codebase
+	$(GOLANGCI_LINT) run -v --fast=false --fix
+
+.PHONY: modules
+modules:
+	go mod tidy
+	go mod vendor
+
+.PHONY: test
+test:
+	go test ./...
+
 .PHONY: ALWAYS
 ALWAYS:

openshift/cmd/manager.go

@@ -20,28 +20,25 @@ import (
 	"flag"
 	"os"
 
+	openshiftconfig "github.com/openshift/api/config/v1"
+	mapi "github.com/openshift/api/machine/v1beta1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
 	// to ensure that exec-entrypoint and run can make use of them.
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
-
-	"k8s.io/apimachinery/pkg/fields"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
-	//+kubebuilder:scaffold:imports
-
-	openshiftconfig "github.com/openshift/api/config/v1"
-	mapi "github.com/openshift/api/machine/v1beta1"
-	corev1 "k8s.io/api/core/v1"
-
-	"github.com/openshift/cluster-api-provider-openstack/openshift/pkg/infracluster_controller"
+	"github.com/openshift/cluster-api-provider-openstack/openshift/pkg/infraclustercontroller"
 	caposcheme "github.com/openshift/cluster-api-provider-openstack/openshift/pkg/scheme"
+
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
@@ -72,7 +69,7 @@ func main() {
 		HealthProbeBindAddress:  probeAddr,
 		LeaderElection:          enableLeaderElection,
 		LeaderElectionID:        "infracluster-leader-election-capo",
-		LeaderElectionNamespace: infracluster_controller.CAPINamespace,
+		LeaderElectionNamespace: infraclustercontroller.CAPINamespace,
 		// LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily
 		// when the Manager ends. This requires the binary to immediately end when the
 		// Manager is stopped, otherwise, this setting is unsafe. Setting this significantly
@@ -88,28 +85,28 @@ func main() {
 		Cache: cache.Options{
 			// Restrict namespaced watches to the Cluster API namespace
 			DefaultNamespaces: map[string]cache.Config{
-				infracluster_controller.CAPINamespace: {},
+				infraclustercontroller.CAPINamespace: {},
 			},
 
 			ByObject: map[client.Object]cache.ByObject{
 				// MAPI Machines are in their own namespace
 				&mapi.Machine{}: {
 					Namespaces: map[string]cache.Config{
-						infracluster_controller.MAPINamespace: {},
+						infraclustercontroller.MAPINamespace: {},
 					},
 				},
 
 				// We only need to watch a single cluster operator
 				&openshiftconfig.ClusterOperator{}: {
-					Field: fields.OneTermEqualSelector("metadata.name", infracluster_controller.ClusterOperatorName),
+					Field: fields.OneTermEqualSelector("metadata.name", infraclustercontroller.ClusterOperatorName),
 				},
 
 				// We only need to watch a single secret
 				&corev1.Secret{}: {
 					Namespaces: map[string]cache.Config{
-						infracluster_controller.CAPINamespace: {},
+						infraclustercontroller.CAPINamespace: {},
 					},
-					Field: fields.OneTermEqualSelector("metadata.name", infracluster_controller.CredentialsSecretName),
+					Field: fields.OneTermEqualSelector("metadata.name", infraclustercontroller.CredentialsSecretName),
 				},
 			},
 		},
@@ -130,7 +127,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	if err := (&infracluster_controller.OpenShiftClusterReconciler{
+	if err := (&infraclustercontroller.OpenShiftClusterReconciler{
 		Client:       mgr.GetClient(),
 		Recorder:     mgr.GetEventRecorderFor("openshiftcluster-controller"),
 		ScopeFactory: scope.ScopeFactory,

openshift/pkg/infracluster_controller/openstackcluster.go renamed to openshift/pkg/infraclustercontroller/openstackcluster.go

@@ -1,4 +1,4 @@
-package infracluster_controller
+package infraclustercontroller
 
 import (
 	"context"
@@ -280,7 +280,7 @@ func (r *OpenShiftClusterReconciler) ensureInfraCluster(ctx context.Context, log
 	openStackCluster.Spec.Network.ID = defaultSubnet.NetworkID
 	// N.B. Deliberately don't add subnet here: CAPO will use all subnets in network, which should also cover dual stack deployments
 
-	routerID, err := r.getDefaultRouterIDFromSubnet(ctx, log, networkClient, defaultSubnet)
+	routerID, err := r.getDefaultRouterIDFromSubnet(ctx, networkClient, defaultSubnet)
 	if err != nil {
 		return nil, err
 	}
@@ -310,7 +310,7 @@ func (r *OpenShiftClusterReconciler) ensureInfraCluster(ctx context.Context, log
 	return &openStackCluster, nil
 }
 
-func (r *OpenShiftClusterReconciler) getDefaultRouterIDFromSubnet(ctx context.Context, log logr.Logger, networkClient clients.NetworkClient, subnet *subnets.Subnet) (string, error) {
+func (r *OpenShiftClusterReconciler) getDefaultRouterIDFromSubnet(_ context.Context, networkClient clients.NetworkClient, subnet *subnets.Subnet) (string, error) {
 	// Find the port which owns the subnet's gateway IP
 	ports, err := networkClient.ListPort(ports.ListOpts{
 		NetworkID: subnet.NetworkID,
@@ -320,7 +320,7 @@ func (r *OpenShiftClusterReconciler) getDefaultRouterIDFromSubnet(ctx context.Co
 				// XXX: We should search on both subnet and IP
 				// address here, but can't because of
 				// https://github.com/gophercloud/gophercloud/issues/2807
-				//SubnetID:  subnet.ID,
+				// SubnetID:  subnet.ID,
 			},
 		},
 	})

openshift/pkg/scheme/scheme.go

@@ -1,14 +1,14 @@
 package scheme
 
 import (
+	openshiftconfig "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 
-	openshiftconfig "github.com/openshift/api/config/v1"
-	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
 func DefaultScheme() *runtime.Scheme {