Skip to content

add golangci-lint #600

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Jun 13, 2019
Merged

add golangci-lint #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
109 changes: 109 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
---
run:
concurrency: 6
deadline: 5m
skip-files:
- ".*_test\\.go"
Copy link
Contributor

@sgreene570 sgreene570 Jun 12, 2019

Choose a reason for hiding this comment

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

we definitely want to exclude unit test files from linting? Just double checking.

Copy link
Member Author

@runcom runcom Jun 12, 2019

Choose a reason for hiding this comment

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

yep, but as with the other "ignored" linters in this file, we might want, at some point, remove this and fix everything. Right now we need to get this in and fix code (not tests) to have this run and check on every PR w/o asking ppl to gofmt as you noticed.

Copy link
Contributor

@sgreene570 sgreene570 Jun 12, 2019

Choose a reason for hiding this comment

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

ok sounds good

- "lib/.*"
linters:
disable-all: true
enable:
- deadcode
- depguard
#- dupl
#- gochecknoinits
- goconst
- gocritic
- gofmt
- goimports
- golint
- gosec
- govet
- ineffassign
#- interfacer
#- maligned
- misspell
#- prealloc
#- scopelint
- staticcheck
- structcheck
- typecheck
- unconvert
- unparam
- varcheck
#- errcheck
#- gochecknoglobals
- gocyclo
#- lll
#- nakedret
linters-settings:
gocritic:
enabled-checks:
# Diagnostic
- argOrder
- badCond
- caseOrder
- codegenComment
- commentedOutCode
- deprecatedComment
- dupArg
- dupBranchBody
- dupCase
- dupSubExpr
- exitAfterDefer
- flagDeref
- flagName
- nilValReturn
- offBy1
- weakCond
#- appendAssign
- octalLiteral
- sloppyReassign

# Performance
- equalFold
#- hugeParam
- indexAlloc
- rangeExprCopy
#- rangeValCopy
- appendCombine

# Style
- assignOp
- boolExprSimplify
- captLocal
- commentFormatting
- commentedOutImport
- defaultCaseOrder
- docStub
- elseif
- emptyFallthrough
- emptyStringTest
- hexLiteral
#- ifElseChain
- methodExprCall
- regexpMust
- singleCaseSwitch
- sloppyLen
- stringXbytes
- switchTrue
- typeAssertChain
- typeSwitchVar
- underef
- unlabelStmt
- unlambda
- unslice
- valSwap
- yodaStyleExpr
- wrapperFunc

# Opinionated
- initClause
- nestingReduce
- ptrToRefParam
- typeUnparen
- unnecessaryBlock
#- builtinShadow
#- importShadow
- paramTypeCombine
#- unnamedResult
2 changes: 2 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ update:
# make verify
verify:
@which go-bindata 2> /dev/null >&1 || { echo "go-bindata must be installed to verify generated code"; exit 1; }
@which golangci-lint 2> /dev/null >&1 || { echo "golangci-lint must be installed to lint code"; exit 1; }
Copy link
Contributor

@DanyC97 DanyC97 May 15, 2019

Choose a reason for hiding this comment

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

@runcom qq - is it bound to a particular release ?

Copy link
Contributor

@DanyC97 DanyC97 May 15, 2019

Choose a reason for hiding this comment

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

i suspect it is by looking https://github.com/openshift/release/pull/3799/files so maybe the echo can be extended to mention that ... my 0.02$ though

Copy link
Member Author

@runcom runcom Jun 12, 2019

Choose a reason for hiding this comment

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

closed that PR as it's not needed now that we're all above go1.10 (1.12 to be precise)

Copy link

@ericavonb ericavonb Jun 12, 2019

Choose a reason for hiding this comment

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

How about adding a target to install it for you? e.g.

$(GOBIN)/golangci-lint: 
	go get -u github.com/golangci/golangci-lint/cmd/golangci-lint

verify: $(GOBIN)/golangci-lint

Copy link
Member Author

@runcom runcom Jun 12, 2019

Choose a reason for hiding this comment

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

I've gona back and forth on the target (we used to have it in CRI-O as well but ended up being dropped cause the CI was taking care of it https://github.com/cri-o/cri-o/pull/2247/files#diff-b67911656ef5d18c4ae36cb6741b7965L308)

The real pain with go1.12 and onwards would be modules also (and calculating $GOBIN). I'm not yet familiar with that. I'm super fine following up to this and add targets to install tools also 👍

Copy link

@ericavonb ericavonb Jun 12, 2019

Choose a reason for hiding this comment

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

👍 I guess I'm used to having a golang version check to handle compatibility. there were issues with GOBIN? never come across that myself.

Copy link
Member Author

@runcom runcom Jun 12, 2019

Choose a reason for hiding this comment

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

I guess I'm used to having a golang version check to handle compatibility.

oh cool, that might be something useful to have here in MCO as well :)

there were issues with GOBIN? never come across that myself.

afaict with go modules, go build creates binaries in PWD so that must be conditionalized as well (hence your suggestion above to version check is amazing)

golangci-lint run
hack/verify-codegen.sh
hack/verify-generated-bindata.sh

Expand Down
23 changes: 1 addition & 22 deletions cmd/machine-config-operator/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ package main

import (
"flag"
"fmt"
"io/ioutil"

"github.com/golang/glog"
"github.com/spf13/cobra"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"

"github.com/openshift/machine-config-operator/pkg/operator"
"github.com/openshift/machine-config-operator/pkg/version"
Expand Down Expand Up @@ -103,25 +98,9 @@ func runBootstrapCmd(cmd *cobra.Command, args []string) {
bootstrapOpts.infraConfigFile, bootstrapOpts.networkConfigFile,
bootstrapOpts.cloudConfigFile,
bootstrapOpts.etcdCAFile, bootstrapOpts.etcdMetricCAFile, bootstrapOpts.rootCAFile, bootstrapOpts.kubeCAFile, bootstrapOpts.pullSecretFile,
imgs,
&imgs,
bootstrapOpts.destinationDir,
); err != nil {
glog.Fatalf("error rendering bootstrap manifests: %v", err)
}
}

func rawImagesFromConfigMapOnDisk(file string) ([]byte, error) {
data, err := ioutil.ReadFile(bootstrapOpts.imagesConfigMapFile)
if err != nil {
return nil, err
}
obji, err := runtime.Decode(scheme.Codecs.UniversalDecoder(corev1.SchemeGroupVersion), data)
if err != nil {
return nil, err
}
cm, ok := obji.(*corev1.ConfigMap)
if !ok {
return nil, fmt.Errorf("expected *corev1.ConfigMap found %T", obji)
}
return []byte(cm.Data["images.json"]), nil
}
3 changes: 0 additions & 3 deletions cmd/machine-config-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ var (
Short: "Run Machine Config Operator",
Long: "",
}

rootOpts struct {
}
)

func init() {
Expand Down
2 changes: 1 addition & 1 deletion internal/clients/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
)

// Builder can create a variety of kubernetes client interface
// with its embeded rest.Config.
// with its embedded rest.Config.
type Builder struct {
config *rest.Config
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
yamlutil "k8s.io/apimachinery/pkg/util/yaml"
kscheme "k8s.io/client-go/kubernetes/scheme"

"github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
v1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
"github.com/openshift/machine-config-operator/pkg/controller/render"
"github.com/openshift/machine-config-operator/pkg/controller/template"
"github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/scheme"
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/common/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package common

import (
"io/ioutil"

igntypes "github.com/coreos/ignition/config/v2_2/types"
"github.com/golang/glog"
"io/ioutil"
)

// NewIgnConfig returns an empty ignition config with version set as latest version
Expand Down
19 changes: 10 additions & 9 deletions pkg/controller/container-runtime-config/container_runtime_config_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,15 +194,15 @@ func (ctrl *Controller) imageConfAdded(obj interface{}) {
ctrl.imgQueue.Add("openshift-config")
}

func (ctrl *Controller) imageConfUpdated(oldObj interface{}, newObj interface{}) {
func (ctrl *Controller) imageConfUpdated(oldObj, newObj interface{}) {
ctrl.imgQueue.Add("openshift-config")
}

func (ctrl *Controller) imageConfDeleted(obj interface{}) {
ctrl.imgQueue.Add("openshift-config")
}

func (ctrl *Controller) updateContainerRuntimeConfig(oldObj interface{}, newObj interface{}) {
func (ctrl *Controller) updateContainerRuntimeConfig(oldObj, newObj interface{}) {
oldCtrCfg := oldObj.(*mcfgv1.ContainerRuntimeConfig)
newCtrCfg := newObj.(*mcfgv1.ContainerRuntimeConfig)

Expand Down Expand Up @@ -406,7 +406,7 @@ func (ctrl *Controller) syncStatusOnly(cfg *mcfgv1.ContainerRuntimeConfig, err e
_, updateErr := ctrl.client.MachineconfigurationV1().ContainerRuntimeConfigs().UpdateStatus(cfg)
return updateErr
})
// If an error occured in updating the status just log it
// If an error occurred in updating the status just log it
if statusUpdateErr != nil {
glog.Warningf("error updating container runtime config status: %v", statusUpdateErr)
}
Expand Down Expand Up @@ -474,7 +474,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error {
for _, pool := range mcpPools {
role := pool.Name
// Get MachineConfig
managedKey := getManagedKeyCtrCfg(pool, cfg)
managedKey := getManagedKeyCtrCfg(pool)
if err := retry.RetryOnConflict(updateBackoff, func() error {
mc, err := ctrl.client.MachineconfigurationV1().MachineConfigs().Get(managedKey, metav1.GetOptions{})
if err != nil && !errors.IsNotFound(err) {
Expand All @@ -490,13 +490,13 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error {
var storageTOML, crioTOML []byte
ctrcfg := cfg.Spec.ContainerRuntimeConfig
if ctrcfg.OverlaySize != (resource.Quantity{}) {
storageTOML, err = ctrl.mergeConfigChanges(originalStorageIgn, cfg, mc, role, managedKey, isNotFound, updateStorageConfig)
storageTOML, err = ctrl.mergeConfigChanges(originalStorageIgn, cfg, updateStorageConfig)
if err != nil {
glog.V(2).Infoln(cfg, err, "error merging user changes to storage.conf: %v", err)
}
}
if ctrcfg.LogLevel != "" || ctrcfg.PidsLimit != 0 || ctrcfg.LogSizeMax != (resource.Quantity{}) {
crioTOML, err = ctrl.mergeConfigChanges(originalCRIOIgn, cfg, mc, role, managedKey, isNotFound, updateCRIOConfig)
crioTOML, err = ctrl.mergeConfigChanges(originalCRIOIgn, cfg, updateCRIOConfig)
if err != nil {
glog.V(2).Infoln(cfg, err, "error merging user changes to crio.conf: %v", err)
}
Expand All @@ -506,6 +506,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error {
mc = mtmpl.MachineConfigFromIgnConfig(role, managedKey, &tempIgnCfg)
}
mc.Spec.Config = createNewCtrRuntimeConfigIgnition(storageTOML, crioTOML)

mc.SetAnnotations(map[string]string{
ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash,
})
Expand Down Expand Up @@ -535,7 +536,7 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error {

// mergeConfigChanges retrieves the original/default config data from the templates, decodes it and merges in the changes given by the Custom Resource.
// It then encodes the new data and returns it.
func (ctrl *Controller) mergeConfigChanges(origFile *igntypes.File, cfg *mcfgv1.ContainerRuntimeConfig, mc *mcfgv1.MachineConfig, role, managedKey string, isNotFound bool, update updateConfig) ([]byte, error) {
func (ctrl *Controller) mergeConfigChanges(origFile *igntypes.File, cfg *mcfgv1.ContainerRuntimeConfig, update updateConfigFunc) ([]byte, error) {
dataURL, err := dataurl.DecodeString(origFile.Contents.Source)
if err != nil {
return nil, ctrl.syncStatusOnly(cfg, err, "could not decode original Container Runtime config: %v", err)
Expand Down Expand Up @@ -595,7 +596,7 @@ func (ctrl *Controller) syncImageConfig(key string) error {
applied := true
role := pool.Name
// Get MachineConfig
managedKey := getManagedKeyReg(pool, imgcfg)
managedKey := getManagedKeyReg(pool)
if err := retry.RetryOnConflict(updateBackoff, func() error {
// Generate the original registries config
_, _, originalRegistriesIgn, err := ctrl.generateOriginalContainerRuntimeConfigs(role)
Expand Down Expand Up @@ -638,7 +639,7 @@ func (ctrl *Controller) syncImageConfig(key string) error {
ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash,
}
mc.ObjectMeta.OwnerReferences = []metav1.OwnerReference{
metav1.OwnerReference{
{
APIVersion: apicfgv1.SchemeGroupVersion.String(),
Kind: "Image",
Name: imgcfg.Name,
Expand Down
12 changes: 6 additions & 6 deletions pkg/controller/container-runtime-config/container_runtime_config_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ func TestContainerRuntimeConfigCreate(t *testing.T) {
mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0")
mcp2.ObjectMeta.Labels["custom-crio"] = "storage-config"
ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: resource.MustParse("9k"), OverlaySize: resource.MustParse("3G")}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "custom-crio", "my-config"))
mcs1 := helpers.NewMachineConfig(getManagedKeyCtrCfg(mcp, ctrcfg1), map[string]string{"node-role/master": ""}, "dummy://", []igntypes.File{{}})
mcs1 := helpers.NewMachineConfig(getManagedKeyCtrCfg(mcp), map[string]string{"node-role": "master"}, "dummy://", []igntypes.File{{}})

f.ccLister = append(f.ccLister, cc)
f.mcpLister = append(f.mcpLister, mcp)
Expand Down Expand Up @@ -349,7 +349,7 @@ func TestContainerRuntimeConfigUpdate(t *testing.T) {
mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0")
mcp2.ObjectMeta.Labels["custom-crio"] = "storage-config"
ctrcfg1 := newContainerRuntimeConfig("set-log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug", LogSizeMax: resource.MustParse("9k"), OverlaySize: resource.MustParse("3G")}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "custom-crio", "my-config"))
mcs := helpers.NewMachineConfig(getManagedKeyCtrCfg(mcp, ctrcfg1), map[string]string{"node-role/master": ""}, "dummy://", []igntypes.File{{}})
mcs := helpers.NewMachineConfig(getManagedKeyCtrCfg(mcp), map[string]string{"node-role": "master"}, "dummy://", []igntypes.File{{}})

f.ccLister = append(f.ccLister, cc)
f.mcpLister = append(f.mcpLister, mcp)
Expand Down Expand Up @@ -425,8 +425,8 @@ func TestImageConfigCreate(t *testing.T) {
mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0")
imgcfg1 := newImageConfig("cluster", &apicfgv1.RegistrySources{InsecureRegistries: []string{"blah.io"}})
cvcfg1 := newClusterVersionConfig("version", "test.io/myuser/myimage:test")
mcs1 := helpers.NewMachineConfig(getManagedKeyReg(mcp, imgcfg1), map[string]string{"node-role/master": ""}, "dummy://", []igntypes.File{{}})
mcs2 := helpers.NewMachineConfig(getManagedKeyReg(mcp2, imgcfg1), map[string]string{"node-role/worker": ""}, "dummy://", []igntypes.File{{}})
mcs1 := helpers.NewMachineConfig(getManagedKeyReg(mcp), map[string]string{"node-role": "master"}, "dummy://", []igntypes.File{{}})
mcs2 := helpers.NewMachineConfig(getManagedKeyReg(mcp2), map[string]string{"node-role": "worker"}, "dummy://", []igntypes.File{{}})

f.ccLister = append(f.ccLister, cc)
f.mcpLister = append(f.mcpLister, mcp)
Expand Down Expand Up @@ -457,8 +457,8 @@ func TestImageConfigUpdate(t *testing.T) {
mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0")
imgcfg1 := newImageConfig("cluster", &apicfgv1.RegistrySources{InsecureRegistries: []string{"blah.io"}})
cvcfg1 := newClusterVersionConfig("version", "test.io/myuser/myimage:test")
mcs1 := helpers.NewMachineConfig(getManagedKeyReg(mcp, imgcfg1), map[string]string{"node-role/master": ""}, "dummy://", []igntypes.File{{}})
mcs2 := helpers.NewMachineConfig(getManagedKeyReg(mcp2, imgcfg1), map[string]string{"node-role/worker": ""}, "dummy://", []igntypes.File{{}})
mcs1 := helpers.NewMachineConfig(getManagedKeyReg(mcp), map[string]string{"node-role": "master"}, "dummy://", []igntypes.File{{}})
mcs2 := helpers.NewMachineConfig(getManagedKeyReg(mcp2), map[string]string{"node-role": "worker"}, "dummy://", []igntypes.File{{}})

f.ccLister = append(f.ccLister, cc)
f.mcpLister = append(f.mcpLister, mcp)
Expand Down
12 changes: 7 additions & 5 deletions pkg/controller/container-runtime-config/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const (
registriesConfigPath = "/etc/containers/registries.conf"
)

var errParsingReference error = errors.New("error parsing reference of desired image from cluster version config")
var errParsingReference = errors.New("error parsing reference of desired image from cluster version config")

// TOML-friendly explicit tables used for conversions.
type tomlConfigStorage struct {
Expand All @@ -55,11 +55,11 @@ type tomlConfigCRIO struct {

type tomlConfigRegistries struct {
Registries []sysregistriesv2.Registry `toml:"registry"`
// backwards compatability to sysregistries v1
// backwards compatibility to sysregistries v1
sysregistriesv2.V1TOMLConfig `toml:"registries"`
}

type updateConfig func(data []byte, internal *mcfgv1.ContainerRuntimeConfiguration) ([]byte, error)
type updateConfigFunc func(data []byte, internal *mcfgv1.ContainerRuntimeConfiguration) ([]byte, error)

func createNewCtrRuntimeConfigIgnition(storageTOMLConfig, crioTOMLConfig []byte) igntypes.Config {
tempIgnConfig := ctrlcommon.NewIgnConfig()
Expand Down Expand Up @@ -132,6 +132,7 @@ func createNewRegistriesConfigIgnition(registriesTOMLConfig []byte) igntypes.Con
func findStorageConfig(mc *mcfgv1.MachineConfig) (*igntypes.File, error) {
for _, c := range mc.Spec.Config.Storage.Files {
if c.Path == storageConfigPath {
c := c
return &c, nil
}
}
Expand All @@ -141,6 +142,7 @@ func findStorageConfig(mc *mcfgv1.MachineConfig) (*igntypes.File, error) {
func findCRIOConfig(mc *mcfgv1.MachineConfig) (*igntypes.File, error) {
for _, c := range mc.Spec.Config.Storage.Files {
if c.Path == crioConfigPath {
c := c
return &c, nil
}
}
Expand All @@ -156,11 +158,11 @@ func findRegistriesConfig(mc *mcfgv1.MachineConfig) (*igntypes.File, error) {
return nil, fmt.Errorf("could not find Registries Config")
}

func getManagedKeyCtrCfg(pool *mcfgv1.MachineConfigPool, config *mcfgv1.ContainerRuntimeConfig) string {
func getManagedKeyCtrCfg(pool *mcfgv1.MachineConfigPool) string {
return fmt.Sprintf("99-%s-%s-containerruntime", pool.Name, pool.ObjectMeta.UID)
}

func getManagedKeyReg(pool *mcfgv1.MachineConfigPool, config *apicfgv1.Image) string {
func getManagedKeyReg(pool *mcfgv1.MachineConfigPool) string {
return fmt.Sprintf("99-%s-%s-registries", pool.Name, pool.ObjectMeta.UID)
}

Expand Down
1 change: 1 addition & 0 deletions pkg/controller/kubelet-config/kubelet_config_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ func (ctrl *Controller) syncKubeletConfig(key string) error {
mc = mtmpl.MachineConfigFromIgnConfig(role, managedKey, &ignConfig)
}
mc.Spec.Config = createNewKubeletIgnition(cfgYAML)

mc.SetAnnotations(map[string]string{
ctrlcommon.GeneratedByControllerVersionAnnotationKey: version.Hash,
})
Expand Down
Loading