chore(fmt): Enable disabled linters in global scope (#238)

* chore(fmt): Enable disabled linters in global scope

Global options:
* An explanation for all nolint annotations is required.
  Please do not just ignore it, tell us why is it ignored.

== wsl

* Fixed everywhere.

== lll

* Fix everywhere.
* Added exception on `https://` links.

== gochecknoglobals

* Fixed everywhere.

== gci

* Fixed everywhere (with `golangci-lint run --fix`)

== godox

No TODO comments should live in the code without filed issues to track
them. The reason is simple: if we have a comment with "todo", it has the
same value as not having that comment at all, because no one will care
about it.

* No TODO, BUG, and FIXME comments in the code.
* Except if it has a filed github issue reference.

* Update golangci-lint and fix new issues

Disabled linters:

golint:
  The linter 'golint' is deprecated (since v1.41.0) due to: The
  repository of the linter has been archived by the owner.  Replaced by
  revive.

interfacer:
  The linter 'interfacer' is deprecated (since v1.38.0) due to: The
  repository of the linter has been archived by the owner.

scopelint:
  The linter 'scopelint' is deprecated (since v1.39.0) due to: The
  repository of the linter has been deprecated by the owner.  Replaced
  by exportloopref.

maligned:
  fieldalignment

tagliatelle:
  We have a yaml structure already and it uses snake_case keys, this
  linter wants us to replace with camelCase. That would require a design
  document and update all yaml outputs.

ireturn:
  Question?

nilnil:
  We are using nil, nil returns in case it was not an error, but the
  resource does not exist. Later we might want to return an error and
  check for error type on the caller side.

exhaustivestruct:
  We don't want to specify all fields with nil and empty string. I see
  the point, if you specify and the default behavior is changed, it does
  not break the code.

* Use go 1.17 in the lint workflow

.github/workflows/lint.yml

@@ -8,5 +8,8 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '1.17'
       - name: Lint
-        run: make lint
+        run: make lint

.golangci.yml

@@ -6,6 +6,22 @@ run:
       - "./*/mock"
 
 linters-settings:
+  funlen:
+    lines: 110
+    statements: 60
+  staticcheck:
+    go: "1.17"
+  stylecheck:
+    go: "1.17"
+  cyclop:
+    max-complexity: 15
+    skip-tests: true
+  gosec:
+    exclude-generated: true
+  lll:
+    line-length: 120
+  misspell:
+    locale: GB
   goimports:
     local-prefixes: github.com/weaveworks/flintlock
   govet:
@@ -17,6 +33,14 @@ linters-settings:
     allow-unused: false
     require-explanation: true
     require-specific: false
+  varnamelen:
+    ignore-names:
+    - err
+    - wg
+    - fs
+    - id
+    - vm
+    - ns
 
 issues:
   max-same-issues: 0
@@ -28,6 +52,12 @@ issues:
   - text: "local replacement are not allowed: github.com/weaveworks/flintlock/"
     linters:
     - gomoddirectives
+  - text: "sig: func github.com/weaveworks/flintlock/"
+    linters:
+    - wrapcheck
+  - source: "https://"
+    linters:
+    - lll
   - path: _test\.go
     linters:
     - goerr113
@@ -45,73 +75,23 @@ issues:
     linters:
     - exhaustivestruct
     - lll
-
-linters-settings:
-  funlen:
-    lines: 110
-    statements: 60
-  staticcheck:
-    go: "1.17"
-  stylecheck:
-    go: "1.17"
-  cyclop:
-    max-complexity: 12
-    skip-tests: true
+    - wrapcheck
+  - source: "// .* #\\d+"
+    linters:
+    - godox
+  - path: test/e2e/
+    linters:
+    - goerr113
+    - gomnd
 
 linters:
-  disable-all: true
-  enable:
-  - deadcode
-  - errcheck
-  - gosimple
-  - govet
-  - ineffassign
-  - staticcheck
-  - structcheck
-  - typecheck
-  - unused
-  - varcheck
-  - bodyclose
-  - depguard
-  - dogsled
-  - dupl
-  - exhaustive
-  - exportloopref
-  - funlen
-  - gochecknoinits
-  - gocognit
-  - goconst
-  - gocritic
-  - gocyclo
-  - godot
-  - goerr113
-  - gofmt
-  - gofumpt
-  - goheader
-  - goimports
-  - revive
-  - gomnd
-  - gomodguard
-  - goprintffuncname
-  - gosec
-  - misspell
-  - nakedret
-  - nestif
-  - nlreturn
-  - noctx
-  - nolintlint
-  - prealloc
-  - rowserrcheck
-  - exportloopref
-  - sqlclosecheck
-  - stylecheck
-  - testpackage
-  - unconvert
-  - unparam
-  - whitespace
-  disabled:
-  - gci
-  - godox
-  - gochecknoglobals
-  - lll
-  - wsl
+  enable-all: true
+  disable:
+  - exhaustivestruct
+  - golint
+  - interfacer
+  - ireturn
+  - maligned
+  - nilnil
+  - scopelint
+  - tagliatelle

cmd/dev-helper/main.go

@@ -1,4 +1,4 @@
-//nolint
+//nolint // We don't care about this, it will be deleted.
 package main
 
 import (

core/application/commands.go

@@ -25,10 +25,12 @@ func (a *app) CreateMicroVM(ctx context.Context, mvm *models.MicroVM) (*models.M
 		if err != nil {
 			return nil, fmt.Errorf("generating random name for microvm: %w", err)
 		}
+
 		vmid, err := models.NewVMID(name, defaults.MicroVMNamespace)
 		if err != nil {
 			return nil, fmt.Errorf("creating vmid: %w", err)
 		}
+
 		mvm.ID = *vmid
 	}
 
@@ -49,8 +51,6 @@ func (a *app) CreateMicroVM(ctx context.Context, mvm *models.MicroVM) (*models.M
 		}
 	}
 
-	// TODO: validate the spec
-
 	// Set the timestamp when the VMspec was created.
 	mvm.Spec.CreatedAt = a.ports.Clock().Unix()
 	mvm.Status.State = models.PendingState

core/application/reconcile.go

@@ -18,6 +18,7 @@ func (a *app) ReconcileMicroVM(ctx context.Context, id, namespace string) error
 	logger := log.GetLogger(ctx).WithField("action", "reconcile")
 
 	logger.Debugf("Getting spec for %s/%s", namespace, id)
+
 	spec, err := a.ports.Repo.Get(ctx, ports.RepositoryGetOptions{
 		Name:      id,
 		Namespace: namespace,
@@ -35,8 +36,8 @@ func (a *app) ResyncMicroVMs(ctx context.Context, namespace string) error {
 		"namespace": "ns",
 	})
 	logger.Info("Resyncing specs")
-
 	logger.Debug("Getting all specs")
+
 	specs, err := a.ports.Repo.GetAll(ctx, namespace)
 	if err != nil {
 		return fmt.Errorf("getting all microvm specs for resync: %w", err)

core/models/network.go

@@ -5,7 +5,7 @@ type NetworkInterface struct {
 	// GuestDeviceName is the name of the network interface to create in the microvm.
 	GuestDeviceName string `json:"guest_device_name" validate:"required,excludesall=/@,guestDeviceName"`
 	// AllowMetadataRequests indicates that this interface can be used for metadata requests.
-	// TODO: we may hide this within the firecracker plugin.
+	// TODO: we may hide this within the firecracker plugin. #179
 	AllowMetadataRequests bool `json:"allow_mmds,omitempty"`
 	// GuestMAC allows the specifying of a specifi MAC address to use for the interface. If
 	// not supplied a autogenerated MAC address will be used.
@@ -14,8 +14,6 @@ type NetworkInterface struct {
 	Type IfaceType `json:"type" validate:"oneof=tap macvtap unsupported"`
 	// Address is an optional IP address to assign to this interface. If not supplied then DHCP will be used.
 	Address string `json:"address,omitempty" validate:"omitempty,cidr"`
-	// TODO: add rate limiting.
-	// TODO: add CNI.
 }
 
 type NetworkInterfaceStatus struct {

core/models/vmid.go

@@ -29,6 +29,7 @@ func NewVMID(name, namespace string) (*VMID, error) {
 	if name == "" {
 		return nil, coreerrs.ErrNameRequired
 	}
+
 	if namespace == "" {
 		return nil, coreerrs.ErrNamespaceRequired
 	}
@@ -98,6 +99,7 @@ func splitVMIDFromString(id string) (namespace string, name string, err error) {
 	if parts[0] == "" {
 		return "", "", coreerrs.ErrNamespaceRequired
 	}
+
 	if parts[1] == "" {
 		return "", "", coreerrs.ErrNameRequired
 	}

core/models/volumes.go

@@ -17,7 +17,6 @@ type Volume struct {
 	PartitionID string `json:"partition_id,omitempty"`
 	// Size is the size to resize this volume to.
 	Size int32 `json:"size,omitempty"`
-	// TODO: add rate limiting.
 }
 
 // Volumes represents a collection of volumes.
@@ -38,7 +37,6 @@ func (v Volumes) GetByID(id string) *Volume {
 type VolumeSource struct {
 	// Container is used to specify a source of a volume as a OCI container.
 	Container *ContainerVolumeSource `json:"container,omitempty"`
-	// TODO: add CSI.
 }
 
 // ContainerDriveSource represents the details of a volume coming from a OCI image.

core/plans/microvm_create_update.go

@@ -107,25 +107,32 @@ func (p *microvmCreateOrUpdatePlan) addStep(ctx context.Context, step planner.Pr
 	return nil
 }
 
-func (p *microvmCreateOrUpdatePlan) addImageSteps(ctx context.Context, vm *models.MicroVM, imageSvc ports.ImageService) error {
+func (p *microvmCreateOrUpdatePlan) addImageSteps(ctx context.Context,
+	vm *models.MicroVM,
+	imageSvc ports.ImageService,
+) error {
 	for i := range vm.Spec.Volumes {
 		vol := vm.Spec.Volumes[i]
+
 		status, ok := vm.Status.Volumes[vol.ID]
 		if !ok {
 			status = &models.VolumeStatus{}
 			vm.Status.Volumes[vol.ID] = status
 		}
+
 		if vol.Source.Container != nil {
 			if err := p.addStep(ctx, runtime.NewVolumeMount(&vm.ID, &vol, status, imageSvc)); err != nil {
 				return fmt.Errorf("adding volume mount step: %w", err)
 			}
 		}
 	}
+
 	if string(vm.Spec.Kernel.Image) != "" {
 		if err := p.addStep(ctx, runtime.NewKernelMount(vm, imageSvc)); err != nil {
 			return fmt.Errorf("adding kernel mount step: %w", err)
 		}
 	}
+
 	if vm.Spec.Initrd != nil {
 		if err := p.addStep(ctx, runtime.NewInitrdMount(vm, imageSvc)); err != nil {
 			return fmt.Errorf("adding initrd mount step: %w", err)
@@ -135,14 +142,19 @@ func (p *microvmCreateOrUpdatePlan) addImageSteps(ctx context.Context, vm *model
 	return nil
 }
 
-func (p *microvmCreateOrUpdatePlan) addNetworkSteps(ctx context.Context, vm *models.MicroVM, networkSvc ports.NetworkService) error {
+func (p *microvmCreateOrUpdatePlan) addNetworkSteps(ctx context.Context,
+	vm *models.MicroVM,
+	networkSvc ports.NetworkService,
+) error {
 	for i := range vm.Spec.NetworkInterfaces {
 		iface := vm.Spec.NetworkInterfaces[i]
+
 		status, ok := vm.Status.NetworkInterfaces[iface.GuestDeviceName]
 		if !ok {
 			status = &models.NetworkInterfaceStatus{}
 			vm.Status.NetworkInterfaces[iface.GuestDeviceName] = status
 		}
+
 		if err := p.addStep(ctx, network.NewNetworkInterface(&vm.ID, &iface, status, networkSvc)); err != nil {
 			return fmt.Errorf("adding create network interface step: %w", err)
 		}

core/plans/microvm_delete.go

@@ -119,7 +119,11 @@ func (p *microvmDeletePlan) addStep(ctx context.Context, step planner.Procedure)
 	return nil
 }
 
-func (p *microvmDeletePlan) addNetworkSteps(ctx context.Context, vm *models.MicroVM, networkSvc ports.NetworkService) error {
+func (p *microvmDeletePlan) addNetworkSteps(
+	ctx context.Context,
+	vm *models.MicroVM,
+	networkSvc ports.NetworkService,
+) error {
 	for i := range vm.Spec.NetworkInterfaces {
 		iface := vm.Spec.NetworkInterfaces[i]
 		ifaceStats := vm.Status.NetworkInterfaces[iface.GuestDeviceName]

core/plans/types.go

@@ -2,6 +2,7 @@ package plans
 
 import (
 	"github.com/spf13/afero"
+
 	"github.com/weaveworks/flintlock/core/ports"
 )
 

core/steps/microvm/create.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/sirupsen/logrus"
+
 	"github.com/weaveworks/flintlock/core/errors"
 	"github.com/weaveworks/flintlock/core/models"
 	"github.com/weaveworks/flintlock/core/ports"

core/steps/microvm/delete.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/sirupsen/logrus"
+
 	"github.com/weaveworks/flintlock/core/errors"
 	"github.com/weaveworks/flintlock/core/models"
 	"github.com/weaveworks/flintlock/core/ports"

core/steps/network/interface_create.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/sirupsen/logrus"
+
 	"github.com/weaveworks/flintlock/core/errors"
 	"github.com/weaveworks/flintlock/core/models"
 	"github.com/weaveworks/flintlock/core/ports"
@@ -13,7 +14,11 @@ import (
 	"github.com/weaveworks/flintlock/pkg/planner"
 )
 
-func NewNetworkInterface(vmid *models.VMID, iface *models.NetworkInterface, status *models.NetworkInterfaceStatus, svc ports.NetworkService) planner.Procedure {
+func NewNetworkInterface(vmid *models.VMID,
+	iface *models.NetworkInterface,
+	status *models.NetworkInterfaceStatus,
+	svc ports.NetworkService,
+) planner.Procedure {
 	return &createInterface{
 		vmid:   vmid,
 		iface:  iface,
@@ -87,10 +92,11 @@ func (s *createInterface) Do(ctx context.Context) ([]planner.Procedure, error) {
 	if err != nil {
 		return nil, fmt.Errorf("checking if networking interface exists: %w", err)
 	}
+
 	if exists {
-		details, err := s.svc.IfaceDetails(ctx, deviceName)
-		if err != nil {
-			return nil, fmt.Errorf("getting interface details: %w", err)
+		details, detailsErr := s.svc.IfaceDetails(ctx, deviceName)
+		if detailsErr != nil {
+			return nil, fmt.Errorf("getting interface details: %w", detailsErr)
 		}
 
 		s.status.HostDeviceName = deviceName

core/steps/network/interface_delete.go

@@ -5,14 +5,18 @@ import (
 	"fmt"
 
 	"github.com/sirupsen/logrus"
+
 	"github.com/weaveworks/flintlock/core/errors"
 	"github.com/weaveworks/flintlock/core/models"
 	"github.com/weaveworks/flintlock/core/ports"
 	"github.com/weaveworks/flintlock/pkg/log"
 	"github.com/weaveworks/flintlock/pkg/planner"
 )
 
-func DeleteNetworkInterface(vmid *models.VMID, iface *models.NetworkInterfaceStatus, svc ports.NetworkService) planner.Procedure {
+func DeleteNetworkInterface(vmid *models.VMID,
+	iface *models.NetworkInterfaceStatus,
+	svc ports.NetworkService,
+) planner.Procedure {
 	return deleteInterface{
 		vmid:  vmid,
 		iface: iface,