From 7b29d11d52d8bdf8949f0f94def07485cd8b88fe Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Tue, 28 Nov 2023 12:18:06 +0000 Subject: [PATCH] openshift: Add make verify and test Also fix lint issues hightlighted by these tests. --- openshift/.golangci.yml | 187 ++++++++++++++++++ openshift/Makefile | 34 +++- openshift/cmd/manager.go | 31 ++- .../openstackcluster.go | 8 +- openshift/pkg/scheme/scheme.go | 6 +- 5 files changed, 239 insertions(+), 27 deletions(-) create mode 100644 openshift/.golangci.yml rename openshift/pkg/{infracluster_controller => infraclustercontroller}/openstackcluster.go (98%) diff --git a/openshift/.golangci.yml b/openshift/.golangci.yml new file mode 100644 index 0000000000..c5ed88e4b4 --- /dev/null +++ b/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 diff --git a/openshift/Makefile b/openshift/Makefile index c768a1639c..669760b5b6 100644 --- a/openshift/Makefile +++ b/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: diff --git a/openshift/cmd/manager.go b/openshift/cmd/manager.go index 4080aeb5bc..2029c57744 100644 --- a/openshift/cmd/manager.go +++ b/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, diff --git a/openshift/pkg/infracluster_controller/openstackcluster.go b/openshift/pkg/infraclustercontroller/openstackcluster.go similarity index 98% rename from openshift/pkg/infracluster_controller/openstackcluster.go rename to openshift/pkg/infraclustercontroller/openstackcluster.go index 38dc8f453c..d1e29f73e7 100644 --- a/openshift/pkg/infracluster_controller/openstackcluster.go +++ b/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, }, }, }) diff --git a/openshift/pkg/scheme/scheme.go b/openshift/pkg/scheme/scheme.go index bfe4f47053..a955bdca49 100644 --- a/openshift/pkg/scheme/scheme.go +++ b/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 {