Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the BMCClient interface, ginkgo, and gomega #102

Merged
merged 6 commits into from
May 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,8 @@ jobs:
- name: make vet
run: make vet

- name: golangci-lint
uses: golangci/golangci-lint-action@v3
with:
args: -v
- name: lint
run: make lint

test:
runs-on: ubuntu-latest
Expand Down
203 changes: 203 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
run:
# The default runtime timeout is 1m, which doesn't work well on Github Actions.
timeout: 4m

# NOTE: This file is populated by the lint-install tool. Local adjustments may be overwritten.
linters-settings:
cyclop:
# NOTE: This is a very high transitional threshold
max-complexity: 37
package-average: 34.0
skip-tests: true

gocognit:
# NOTE: This is a very high transitional threshold
min-complexity: 98

dupl:
threshold: 200

goconst:
min-len: 4
min-occurrences: 5
ignore-tests: true

gosec:
excludes:
- G107 # Potential HTTP request made with variable url
- G204 # Subprocess launched with function call as argument or cmd arguments
- G404 # Use of weak random number generator (math/rand instead of crypto/rand

errorlint:
# these are still common in Go: for instance, exit errors.
asserts: false

exhaustive:
default-signifies-exhaustive: true

nestif:
min-complexity: 8

nolintlint:
require-explanation: true
allow-unused: false
require-specific: true

revive:
ignore-generated-header: true
severity: warning
rules:
- name: atomic
- name: blank-imports
- name: bool-literal-in-expr
- name: confusing-naming
- name: constant-logical-expr
- name: context-as-argument
- name: context-keys-type
- name: deep-exit
- name: defer
- name: range-val-in-closure
- name: range-val-address
- name: dot-imports
- name: error-naming
- name: error-return
- name: error-strings
- name: errorf
- name: exported
- name: identical-branches
- name: if-return
- name: import-shadowing
- name: increment-decrement
- name: indent-error-flow
- name: indent-error-flow
- name: package-comments
- name: range
- name: receiver-naming
- name: redefines-builtin-id
- name: superfluous-else
- name: struct-tag
- name: time-naming
- name: unexported-naming
- name: unexported-return
- name: unnecessary-stmt
- name: unreachable-code
- name: unused-parameter
- name: var-declaration
- name: var-naming
- name: unconditional-recursion
- name: waitgroup-by-value

staticcheck:
go: "1.20"

unused:
go: "1.20"

output:
sort-results: true

linters:
disable-all: true
enable:
- asciicheck
- bodyclose
- cyclop
- dogsled
- dupl
- durationcheck
- errcheck
- errname
- errorlint
- exhaustive
- exportloopref
- forcetypeassert
- gocognit
- goconst
- gocritic
- godot
- gofmt
- gofumpt
- gosec
- goheader
- goimports
- goprintffuncname
- gosimple
- govet
- importas
- ineffassign
- makezero
- misspell
- nakedret
- nestif
- nilerr
- noctx
- nolintlint
- predeclared
# disabling for the initial iteration of the linting tool
# - promlinter
- revive
- rowserrcheck
- sqlclosecheck
- staticcheck
- stylecheck
- thelper
- tparallel
- typecheck
- unconvert
- unparam
- unused
- wastedassign
- whitespace

# Disabled linters, due to being misaligned with Go practices
# - exhaustivestruct
# - gochecknoglobals
# - gochecknoinits
# - goconst
# - godox
# - goerr113
# - gomnd
# - lll
# - nlreturn
# - testpackage
# - wsl
# Disabled linters, due to not being relevant to our code base:
# - maligned
# - prealloc "For most programs usage of prealloc will be a premature optimization."
# Disabled linters due to bad error messages or bugs
# - tagliatelle

issues:
# Excluding configuration per-path, per-linter, per-text and per-source
exclude-rules:
- path: _test\.go
linters:
- dupl
- errcheck
- forcetypeassert
- gocyclo
- gosec
- noctx

- path: .*cmd.*
linters:
- noctx

- path: main\.go
linters:
- noctx

- path: .*cmd.*
text: "deep-exit"

- path: main\.go
text: "deep-exit"

# This check is of questionable value
- linters:
- tparallel
text: "call t.Parallel on the top level as well as its subtests"

# Don't hide lint issues just because there are many of them
max-same-issues: 0
max-issues-per-linter: 0
21 changes: 10 additions & 11 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ endif
SHELL = /usr/bin/env bash -o pipefail
.SHELLFLAGS = -ec

.PHONY: all
all: build

##@ General

# The help target prints out all targets with their descriptions organized
Expand All @@ -37,6 +34,11 @@ all: build
help: ## Display this help.
@awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_0-9-]+:.*?##/ { printf " \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)

include lint.mk

.PHONY: all
all: build

##@ Development

.PHONY: manifests
Expand All @@ -57,7 +59,11 @@ vet: ## Run go vet against code.

.PHONY: test
test: manifests generate ## Run unit tests.
go test ./... -coverprofile cover.out
go test -v ./... -coverprofile cover.out

.PHONY: cover
cover: test ## Run unit tests with coverage report
go tool cover -func=cover.out

.PHONY: integration-test
integration-test: manifests generate fmt vet envtest ## Run integration tests.
Expand Down Expand Up @@ -119,13 +125,6 @@ ENVTEST = $(shell pwd)/bin/setup-envtest
envtest: ## Download envtest-setup locally if necessary.
$(call go-get-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@latest)

MOCKGEN = $(shell pwd)/bin/mockgen
.PHONY: mocks
mocks: ## Generate mocks
$(call go-get-tool,$(MOCKGEN),github.com/golang/mock/mockgen@v1.6.0)
${MOCKGEN} -destination=controllers/mocks/bmcclient.go -package=mocks "github.com/tinkerbell/rufio/controllers" BMCClient


##@ Release

RELEASE_TAG := $(shell git describe --abbrev=0 2>/dev/null)
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Rufio

![For each commit and PR](https://github.com/tinkerbell/rufio/workflows/For%20each%20commit%20and%20PR/badge.svg)
[![codecov](https://codecov.io/gh/tinkerbell/rufio/branch/main/graph/badge.svg)](https://codecov.io/gh/tinkerbell/rufio)

## Description

Expand Down
4 changes: 2 additions & 2 deletions api/v1alpha1/groupversion_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ import (
)

var (
// GroupVersion is group version used to register these objects
// GroupVersion is group version used to register these objects.
GroupVersion = schema.GroupVersion{Group: "bmc.tinkerbell.org", Version: "v1alpha1"}

// SchemeBuilder is used to add go types to the GroupVersionKind scheme
// SchemeBuilder is used to add go types to the GroupVersionKind scheme.
SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion}

// AddToScheme adds the types in this group-version to the given scheme.
Expand Down
12 changes: 6 additions & 6 deletions api/v1alpha1/job.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type MachineRef struct {
Namespace string `json:"namespace"`
}

// JobSpec defines the desired state of Job
// JobSpec defines the desired state of Job.
type JobSpec struct {
// MachineRef represents the Machine resource to execute the job.
// All the tasks in the job are executed for the same Machine.
Expand All @@ -58,7 +58,7 @@ type JobSpec struct {
Tasks []Action `json:"tasks"`
}

// JobStatus defines the observed state of Job
// JobStatus defines the observed state of Job.
type JobStatus struct {
// Conditions represents the latest available observations of an object's current state.
// +optional
Expand Down Expand Up @@ -144,9 +144,9 @@ func FormatTaskName(job Job, n int) string {
//+kubebuilder:subresource:status
//+kubebuilder:resource:path=jobs,scope=Namespaced,categories=tinkerbell,singular=job,shortName=j

// Job is the Schema for the bmcjobs API
// Job is the Schema for the bmcjobs API.
type Job struct {
metav1.TypeMeta `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm weary of removing inline because every object I've ever seen does it. I'm not 100%, but I think its an option used by Kubernetes client-go json/yaml implementation. Its also documented in examples in the kubebuilder book https://book.kubebuilder.io/cronjob-tutorial/new-api.html.

Copy link
Member Author

@jacobweinstock jacobweinstock May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline is not a recognized struct tag in Go. I believe the only reason to keep them is if we have any code generation that depends on it. I ran make generate and make manifests and don't think I saw any issues. I could be mistaken though.

ref:
golang/go#6213
kubernetes/kubernetes#117206

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this has plagued me with the linting and I could never find anything useful about it.

metav1.TypeMeta `json:""`
metav1.ObjectMeta `json:"metadata,omitempty"`

Spec JobSpec `json:"spec,omitempty"`
Expand All @@ -155,9 +155,9 @@ type Job struct {

//+kubebuilder:object:root=true

// JobList contains a list of Job
// JobList contains a list of Job.
type JobList struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:""`
metav1.ListMeta `json:"metadata,omitempty"`
Items []Job `json:"items"`
}
Expand Down
10 changes: 5 additions & 5 deletions api/v1alpha1/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const (
ConditionFalse ConditionStatus = "False"
)

// MachineSpec defines desired machine state
// MachineSpec defines desired machine state.
type MachineSpec struct {
// Connection contains connection data for a Baseboard Management Controller.
Connection Connection `json:"connection"`
Expand All @@ -69,7 +69,7 @@ type Connection struct {
InsecureTLS bool `json:"insecureTLS"`
}

// MachineStatus defines the observed state of Machine
// MachineStatus defines the observed state of Machine.
type MachineStatus struct {
// Power is the current power state of the Machine.
// +kubebuilder:validation:Enum=on;off
Expand Down Expand Up @@ -142,9 +142,9 @@ func WithMachineConditionMessage(m string) MachineSetConditionOption {
//+kubebuilder:subresource:status
//+kubebuilder:resource:path=machines,scope=Namespaced,categories=tinkerbell,singular=machine

// Machine is the Schema for the machines API
// Machine is the Schema for the machines API.
type Machine struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:""`
metav1.ObjectMeta `json:"metadata,omitempty"`

Spec MachineSpec `json:"spec,omitempty"`
Expand All @@ -155,7 +155,7 @@ type Machine struct {

// MachineList contains a list of Machines.
type MachineList struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:""`
metav1.ListMeta `json:"metadata,omitempty"`
Items []Machine `json:"items"`
}
Expand Down
8 changes: 4 additions & 4 deletions api/v1alpha1/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ type Action struct {
VirtualMediaAction *VirtualMediaAction `json:"virtualMediaAction,omitempty"`
}

// TaskStatus defines the observed state of Task
// TaskStatus defines the observed state of Task.
type TaskStatus struct {
// Conditions represents the latest available observations of an object's current state.
// +optional
Expand Down Expand Up @@ -138,7 +138,7 @@ func (t *Task) HasCondition(cType TaskConditionType, cStatus ConditionStatus) bo

// Task is the Schema for the Task API.
type Task struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:""`
metav1.ObjectMeta `json:"metadata,omitempty"`

Spec TaskSpec `json:"spec,omitempty"`
Expand All @@ -147,9 +147,9 @@ type Task struct {

//+kubebuilder:object:root=true

// TaskList contains a list of Task
// TaskList contains a list of Task.
type TaskList struct {
metav1.TypeMeta `json:",inline"`
metav1.TypeMeta `json:""`
metav1.ListMeta `json:"metadata,omitempty"`
Items []Task `json:"items"`
}
Expand Down
Loading