Add additional checks to golangci-lint (#5567) (#5687)

* Document golangci-lint configuration options

* Do not limit the number of issues reported per linter

* Add check preventing the use of the deprecated 'github.com/pkg/errors' package

* Upgrade golangci-lint to 1.37.0, so we can use 'revive' Linter

- revive is a drop-in replacement for the deprecated golint Linter
- revive will allow to check for error strings

Note: We are purposely not using the latest golangci-lint (1.45.0)
but the minimum version from which revive is available.
This is because the latest 1.45.0 reports additional linting issues
(from govet and staticcheck for example). And fixing those side errors
is outside of the scope of this issue.
Also some of those issues are already fixed in #5497 (update to Go 1.17).

* Configure revive to check for error strings

More rules can be added later on if needed

* Fix issues reported by revive's error-strings rule

Some rules are purposely ignored when the
error messages represent top-level errors that are
displayed to be displayed as is to end users

* Fix more error-strings issues

For some reason, those were not reported by revive's error-strings rule,
but only by GoLand inspection tool.

* Fix missing `revive:disable:error-strings` comment directive

Also replace "fmt.Errorf" by "errors.New" when the error message is static
This commit is contained in:
Armel Soro
2022-04-25 13:32:20 +02:00
committed by GitHub
parent 54374b209a
commit 06550eb233
29 changed files with 131 additions and 47 deletions

View File

@@ -2,14 +2,78 @@
# https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml
run:
# Timeout for analysis, e.g. 30s, 5m.
# Default: 1m
timeout: 5m
# If set we pass it to "go list -mod={option}". From "go help modules":
# If invoked with -mod=readonly, the go command is disallowed from the implicit
# automatic updating of go.mod described above. Instead, it fails when any changes
# to go.mod are needed. This setting is most useful to check that go.mod does
# not need updates, such as in a continuous integration and testing system.
# If invoked with -mod=vendor, the go command assumes that the vendor
# directory holds the correct copies of dependencies and ignores
# the dependency descriptions in go.mod.
#
# Allowed values: readonly|vendor|mod
# By default, it isn't set.
modules-download-mode: readonly
linters:
# Note that some linters not listed below are enabled by default.
# See https://golangci-lint.run/usage/linters/#enabled-by-default
enable:
# Go linter that checks if package imports are in a list of acceptable packages
- depguard
# check whether code was gofmt-ed. By default, this tool runs with -s option to check for code simplification
- gofmt
# analyze code for common mistakes
- govet
# Fast, configurable, extensible, flexible, and beautiful linter for Go.
# Drop-in replacement of golint, which has been deprecated
- revive
linters-settings:
depguard:
# A list of packages for the list type specified. Default list type is 'denylist'
# Default: []
packages:
- github.com/pkg/errors
# A list of packages for the list type specified.
# Specify an error message to output when a denied package is used.
# Default: []
packages-with-error-message:
- github.com/pkg/errors: >
This package is deprecated since Go 1.13. Use the errors package from the standard library instead.
See https://github.com/redhat-developer/odo/pull/5557 for some examples.
govet:
# Report about shadowed variables.
# Default: false
check-shadowing: true
revive:
# Maximum number of open files at the same time.
# See https://github.com/mgechev/revive#command-line-flags
# Defaults to unlimited.
max-open-files: 2048
# When set to false, ignores files with "GENERATED" header, similar to golint.
# See https://github.com/mgechev/revive#available-rules for details.
# Default: false
ignore-generated-header: true
# Sets the default severity.
# See https://github.com/mgechev/revive#configuration
# Default: warning
severity: error
# Explicitly list all rules enabled. We can enable all existing rules by setting 'enable-all-rules' to true.
# See https://github.com/mgechev/revive#available-rules for the available rules.
rules:
# Conventions around error strings. See https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#error-strings
- name: error-strings
issues:
# Maximum issues count per one linter.
# Set to 0 to disable.
# Default: 50
max-issues-per-linter: 0
# Maximum count of issues with the same text.
# Set to 0 to disable.
# Default: 3
max-same-issues: 0

View File

@@ -111,7 +111,7 @@ clean:
.PHONY: goget-tools
goget-tools:
mkdir -p $(shell go env GOPATH)/bin
curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin v1.30.0
curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell go env GOPATH)/bin v1.37.0
.PHONY: goget-ginkgo
goget-ginkgo:

View File

@@ -96,7 +96,7 @@ func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj pars
var deploymentName string
deploymentName, err = util.NamespaceKubernetesObject(componentName, appName)
if err != nil {
return isInnerLoopDeployed, resources, fmt.Errorf("Failed to get the resource %q name for component %q; cause: %w", kclient.DeploymentKind, deploymentName, err)
return isInnerLoopDeployed, resources, fmt.Errorf("failed to get the resource %q name for component %q; cause: %w", kclient.DeploymentKind, deploymentName, err)
}
deployment, err := do.kubeClient.GetDeploymentByName(deploymentName)
@@ -112,7 +112,7 @@ func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj pars
var unstructuredDeploy unstructured.Unstructured
unstructuredDeploy, err = kclient.ConvertK8sResourceToUnstructured(deployment)
if err != nil {
return isInnerLoopDeployed, resources, fmt.Errorf("Failed to parse the resource %q: %q; cause: %w", kclient.DeploymentKind, deploymentName, err)
return isInnerLoopDeployed, resources, fmt.Errorf("failed to parse the resource %q: %q; cause: %w", kclient.DeploymentKind, deploymentName, err)
}
resources = append(resources, unstructuredDeploy)
}
@@ -121,7 +121,7 @@ func (do DeleteComponentClient) ListResourcesToDeleteFromDevfile(devfileObj pars
// Parse the devfile for outerloop K8s resources
localResources, err := libdevfile.ListKubernetesComponents(devfileObj, devfileObj.Ctx.GetAbsPath())
if err != nil {
return isInnerLoopDeployed, resources, fmt.Errorf("Failed to gather resources for deletion: %w", err)
return isInnerLoopDeployed, resources, fmt.Errorf("failed to gather resources for deletion: %w", err)
}
for _, lr := range localResources {
var gvr *meta.RESTMapping

View File

@@ -40,7 +40,7 @@ func DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedT
if contextDir == "" {
path, err = os.Getwd()
if err != nil {
return fmt.Errorf("Could not get the current working directory: %w", err)
return fmt.Errorf("could not get the current working directory: %w", err)
}
} else {
path = contextDir
@@ -81,7 +81,7 @@ func DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedT
downloadSpinner.End(true)
}
} else {
return errors.New("Project type not supported")
return errors.New("project type not supported")
}
return nil

View File

@@ -101,5 +101,5 @@ func (o *deployHandler) Execute(command v1alpha2.Command) error {
// TODO:
// * Make sure we inject the "deploy" mode label once we implement exec in `odo deploy`
// * Make sure you inject the "component type" label once we implement exec.
return errors.New("Exec command is not implemented for Deploy")
return errors.New("exec command is not implemented for Deploy")
}

View File

@@ -1,8 +1,7 @@
package adapters
import (
"fmt"
"errors"
devfileParser "github.com/devfile/library/pkg/devfile/parser"
"github.com/redhat-developer/odo/pkg/devfile/adapters/common"
"github.com/redhat-developer/odo/pkg/devfile/adapters/kubernetes"
@@ -22,7 +21,7 @@ func NewComponentAdapter(componentName string, context string, appName string, d
kc, ok := platformContext.(kubernetes.KubernetesContext)
if !ok {
return nil, fmt.Errorf("Error retrieving context for Kubernetes")
return nil, errors.New("error retrieving context for Kubernetes")
}
return createKubernetesAdapter(adapterContext, kc.Namespace)

View File

@@ -35,7 +35,7 @@ func (k Adapter) Push(parameters common.PushParameters) error {
err := k.componentAdapter.Push(parameters)
if err != nil {
return fmt.Errorf("Failed to create the component: %w", err)
return fmt.Errorf("failed to create the component: %w", err)
}
return nil
@@ -45,7 +45,7 @@ func (k Adapter) Push(parameters common.PushParameters) error {
func (k Adapter) CheckSupervisordCommandStatus(command devfilev1.Command) error {
err := k.componentAdapter.CheckSupervisordCommandStatus(command)
if err != nil {
return fmt.Errorf("Failed to check the status: %w", err)
return fmt.Errorf("failed to check the status: %w", err)
}
return nil

View File

@@ -269,7 +269,7 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
a.deployment, err = a.Client.WaitForDeploymentRollout(a.deployment.Name)
if err != nil {
return fmt.Errorf("Failed to update config to component deployed: %w", err)
return fmt.Errorf("failed to update config to component deployed: %w", err)
}
// Wait for Pod to be in running state otherwise we can't sync data or exec commands to it.
@@ -311,7 +311,7 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
execRequired, err := syncAdapter.SyncFiles(syncParams)
if err != nil {
return fmt.Errorf("Failed to sync to component with name %s: %w", a.ComponentName, err)
return fmt.Errorf("failed to sync to component with name %s: %w", a.ComponentName, err)
}
s.End(true)

View File

@@ -39,7 +39,7 @@ func GetVolumeInfos(pvcs []corev1.PersistentVolumeClaim) (odoSourcePVCName strin
generatedVolumeName, e := generateVolumeNameFromPVC(pvc.Name)
if e != nil {
return "", nil, fmt.Errorf("Unable to generate volume name from pvc name: %w", e)
return "", nil, fmt.Errorf("unable to generate volume name from pvc name: %w", e)
}
if pvc.Labels[storagelabels.StorageLabel] == storagepkg.OdoSourceVolume {

View File

@@ -121,5 +121,7 @@ func selectBackend() (Backend, error) {
if _, err := lookPathCmd(dockerCmd); err == nil {
return NewDockerCompatibleBackend(dockerCmd), nil
}
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return nil, errors.New("odo requires either Podman or Docker to be installed in your environment. Please install one of them and try again.")
//revive:enable:error-strings
}

View File

@@ -199,7 +199,7 @@ func (o *InteractiveBackend) PersonalizeDevfileConfig(devfileobj parser.DevfileO
}
case "Nothing":
default:
return zeroDevfile, fmt.Errorf("Unknown configuration selected %q", fmt.Sprintf("%v %v %v", configOps.Ops, configOps.Kind, configOps.Key))
return zeroDevfile, fmt.Errorf("unknown configuration selected %q", fmt.Sprintf("%v %v %v", configOps.Ops, configOps.Kind, configOps.Key))
}
// Update the current configuration
config[selectContainerAnswer] = selectedContainer

View File

@@ -349,7 +349,7 @@ func (c *Client) UnlinkSecret(secretName, componentName, applicationName string)
}
if indexForRemoval == -1 {
return "", fmt.Errorf("Deployment does not contain a link to %s", secretName)
return "", fmt.Errorf("deployment does not contain a link to %s", secretName)
}
return fmt.Sprintf(`[{"op": "remove", "path": "/spec/template/spec/containers/0/envFrom/%d"}]`, indexForRemoval), nil
@@ -372,13 +372,13 @@ func (c *Client) jsonPatchDeployment(deploymentSelector string, deploymentPatchP
if deploymentPatchProvider != nil {
patch, e := deploymentPatchProvider(deployment)
if e != nil {
return fmt.Errorf("Unable to create a patch for the Deployment: %w", e)
return fmt.Errorf("unable to create a patch for the Deployment: %w", e)
}
// patch the Deployment with the secret
_, e = c.KubeClient.AppsV1().Deployments(c.Namespace).Patch(context.TODO(), deployment.Name, types.JSONPatchType, []byte(patch), metav1.PatchOptions{FieldManager: FieldManager})
if e != nil {
return fmt.Errorf("Deployment not patched %s: %w", deployment.Name, e)
return fmt.Errorf("deployment not patched %s: %w", deployment.Name, e)
}
} else {
return errors.New("deploymentPatch was not properly set")

View File

@@ -238,18 +238,18 @@ func (c *Client) WaitForComponentDeletion(selector string) error {
case event, ok := <-eventCh:
_, typeOk := event.Object.(*appsv1.Deployment)
if !ok || !typeOk {
return errors.New("Unable to watch deployments")
return errors.New("unable to watch deployments")
}
if event.Type == watch.Deleted {
klog.V(3).Infof("WaitForComponentDeletion, Event Received:Deleted")
return nil
} else if event.Type == watch.Error {
klog.V(3).Infof("WaitForComponentDeletion, Event Received:Deleted ")
return errors.New("Unable to watch deployments")
return errors.New("unable to watch deployments")
}
case <-time.After(waitForComponentDeletionTimeout):
klog.V(3).Infof("WaitForComponentDeletion, Timeout")
return errors.New("Time out waiting for component to get deleted")
return errors.New("timed out waiting for component to get deleted")
}
}
}

View File

@@ -194,7 +194,7 @@ func (c *Client) WaitForServiceAccountInNamespace(namespace, serviceAccountName
}
}
case <-timeout:
return errors.New("Timed out waiting for service to be ready")
return errors.New("timed out waiting for service to be ready")
}
}
}

View File

@@ -55,7 +55,7 @@ func (c *Client) GetServerVersion(timeout time.Duration) (*ServerInfo, error) {
// checking if the server is reachable
if !isServerUp(config.Host, timeout) {
return nil, errors.New("Unable to connect to OpenShift cluster, is it down?")
return nil, errors.New("unable to connect to OpenShift cluster, it may be down")
}
// fail fast if user is not connected (same logic as `oc whoami`)

View File

@@ -1,6 +1,7 @@
package cli
import (
"errors"
"flag"
"fmt"
"os"
@@ -257,7 +258,9 @@ func ShowSubcommands(cmd *cobra.Command, args []string) error {
strs = append(strs, subcmd.Name())
}
}
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return fmt.Errorf("Subcommand not found, use one of the available commands: %s", strings.Join(strs, ", "))
//revive:enable:error-strings
}
// ShowHelp will show the help correctly (and whether or not the command is invalid...)
@@ -276,5 +279,7 @@ func ShowHelp(cmd *cobra.Command, args []string) error {
return nil
}
return fmt.Errorf("Invalid command - see available commands/subcommands above")
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return errors.New("Invalid command - see available commands/subcommands above")
//revive:enable:error-strings
}

View File

@@ -2,6 +2,8 @@ package registry
import (
"context"
"errors"
// Built-in packages
"fmt"
"io"
@@ -63,7 +65,9 @@ func (o *ListOptions) Validate() (err error) {
func (o *ListOptions) Run(ctx context.Context) (err error) {
registryList := o.clientset.PreferenceClient.RegistryList()
if registryList == nil || len(*registryList) == 0 {
return fmt.Errorf("No devfile registries added to the configuration. Refer `odo preference registry add -h` to add one")
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return errors.New("No devfile registries added to the configuration. Refer `odo preference registry add -h` to add one")
//revive:enable:error-strings
}
w := tabwriter.NewWriter(os.Stdout, 5, 2, 3, ' ', tabwriter.TabIndent)

View File

@@ -71,7 +71,9 @@ func (pdo *ProjectDeleteOptions) Validate() error {
return &odoerrors.Unauthorized{}
}
if !isValidProject {
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return fmt.Errorf("The project %q does not exist. Please check the list of projects using `odo project list`", pdo.projectName)
//revive:enable:error-strings
}
return nil
}

View File

@@ -77,7 +77,9 @@ func (pso *ProjectSetOptions) Validate() (err error) {
return &odoerrors.Unauthorized{}
}
if !exists {
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return fmt.Errorf("The project %s does not exist", pso.projectName)
//revive:enable:error-strings
}
return nil

View File

@@ -2,8 +2,7 @@ package cmdline
import (
"context"
"fmt"
"errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
@@ -30,7 +29,7 @@ func (o *Cobra) Context() context.Context {
func (o *Cobra) GetArgsAfterDashes(args []string) ([]string, error) {
l := o.cmd.ArgsLenAtDash()
if l < 0 {
return nil, fmt.Errorf("no argument passed after dash")
return nil, errors.New("no argument passed after dash")
}
return args[l:], nil
}
@@ -98,7 +97,7 @@ func (o *Cobra) CheckIfConfigurationNeeded() (bool, error) {
// This should *never* happen, but added just to be safe
if firstChildCommand == nil {
return false, fmt.Errorf("Unable to get first child of command")
return false, errors.New("unable to get first child of command")
}
// Gather necessary preliminary information

View File

@@ -168,7 +168,7 @@ func (o *Context) ComponentAllowingEmpty(allowEmpty bool, optionalComponent ...s
// if we're not specifying a component to resolve, get the current one (resolved in NewContext as cmp)
// so nothing to do here unless the calling context doesn't allow no component to be set in which case we return an error
if !allowEmpty && len(o.component) == 0 {
return "", fmt.Errorf("No component is set")
return "", errors.New("no component is set")
}
case 1:
cmp := optionalComponent[0]

View File

@@ -1,7 +1,7 @@
package genericclioptions
import (
"fmt"
"errors"
"github.com/redhat-developer/odo/pkg/envinfo"
"github.com/redhat-developer/odo/pkg/odo/cmdline"
@@ -33,10 +33,12 @@ func GetValidEnvInfo(cmdline cmdline.Cmdline) (*envinfo.EnvSpecificInfo, error)
// Check to see if the environment file exists
if !envInfo.Exists() {
return nil, fmt.Errorf(`The current directory does not represent an odo component.
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return nil, errors.New(`The current directory does not represent an odo component.
To start editing your component, use "odo dev" and open this folder in your favorite IDE. Changes will be directly reflected on the cluster.
To deploy your component to a cluster use "odo deploy".
Or switch to directory with a component.`)
//revive:enable:error-strings
}
return envInfo, nil

View File

@@ -106,7 +106,7 @@ func (o *internalCxt) checkComponentExistsOrFail() error {
return err
}
if !exists {
return fmt.Errorf("Component %v does not exist in application %s", o.component, o.application)
return fmt.Errorf("component %v does not exist in application %s", o.component, o.application)
}
return nil
}

View File

@@ -3,6 +3,7 @@ package genericclioptions
import (
"context"
"encoding/json"
"errors"
"flag"
"fmt"
"os"
@@ -189,7 +190,9 @@ func CheckMachineReadableOutputCommand(cmd *cobra.Command) error {
// Check the valid output
if hasFlagChanged && outputFlag.Value.String() != "json" {
return fmt.Errorf("Please input a valid output format for -o, available format: json")
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return errors.New("Please input a valid output format for -o, available format: json")
//revive:enable:error-strings
}
// Check that if -o json has been passed, that the command actually USES json.. if not, error out.
@@ -199,7 +202,9 @@ func CheckMachineReadableOutputCommand(cmd *cobra.Command) error {
_ = flag.Set("o", "")
// Return the error
return fmt.Errorf("Machine readable output is not yet implemented for this command")
//revive:disable:error-strings This is a top-level error message displayed as is to the end user
return errors.New("Machine readable output is not yet implemented for this command")
//revive:enable:error-strings
}
// Before running anything, we will make sure that no verbose output is made

View File

@@ -41,7 +41,7 @@ const ServiceKind = "app.kubernetes.io/service-kind"
func DeleteOperatorService(client kclient.ClientInterface, serviceName string) error {
kind, name, err := SplitServiceKindName(serviceName)
if err != nil {
return fmt.Errorf("Refer %q to see list of running services: %w", serviceName, err)
return fmt.Errorf("refer %q to see list of running services: %w", serviceName, err)
}
csv, err := client.GetCSVWithCR(kind)
@@ -79,7 +79,7 @@ func ListOperatorServices(client kclient.ClientInterface) ([]unstructured.Unstru
}
if err != nil {
return nil, nil, fmt.Errorf("Unable to list operator backed services: %w", err)
return nil, nil, fmt.Errorf("unable to list operator backed services: %w", err)
}
var allCRInstances []unstructured.Unstructured

View File

@@ -124,7 +124,7 @@ func BindingDefinitions(ctx pipeline.Context) {
for k, v := range anns {
definition, err := makeBindingDefinition(k, v, ctx)
if err != nil {
condition := notCollectionReadyCond(collect.InvalidAnnotation, fmt.Errorf("Failed to create binding definition from \"%v: %v\": %v", k, v, err))
condition := notCollectionReadyCond(collect.InvalidAnnotation, fmt.Errorf("failed to create binding definition from \"%v: %v\": %v", k, v, err))
ctx.SetCondition(condition)
ctx.Error(err)
ctx.StopProcessing()

View File

@@ -157,7 +157,7 @@ func (a Adapter) SyncFiles(syncParameters common.SyncParameters) (bool, error) {
if forceWrite {
err = util.WriteFile(ret.NewFileMap, ret.ResolvedPath)
if err != nil {
return false, fmt.Errorf("Failed to write file: %w", err)
return false, fmt.Errorf("failed to write file: %w", err)
}
}
@@ -240,7 +240,7 @@ func updateIndexWithWatchChanges(pushParameters common.PushParameters) error {
// Parse the existing index
fileIndex, err := util.ReadFileIndex(indexFilePath)
if err != nil {
return fmt.Errorf("Unable to read index from path: %s: %w", indexFilePath, err)
return fmt.Errorf("unable to read index from path: %s: %w", indexFilePath, err)
}
rootDir := pushParameters.Path

View File

@@ -206,7 +206,7 @@ func IsValidProjectDir(path string, devfilePath string) error {
return nil
}
}
return fmt.Errorf("Folder %s doesn't contain the devfile used.", path)
return fmt.Errorf("folder %s doesn't contain the devfile used", path)
}
return nil
@@ -218,7 +218,7 @@ func IsValidProjectDir(path string, devfilePath string) error {
// TODO(feloy) sync with devfile library?
func GetAndExtractZip(zipURL string, destination string, pathToUnzip string, starterToken string) error {
if zipURL == "" {
return fmt.Errorf("Empty zip url: %s", zipURL)
return fmt.Errorf("empty zip url: %s", zipURL)
}
var pathToZip string
@@ -251,7 +251,7 @@ func GetAndExtractZip(zipURL string, destination string, pathToUnzip string, sta
}
}()
} else {
return fmt.Errorf("Invalid Zip URL: %s . Should either be prefixed with file://, http:// or https://", zipURL)
return fmt.Errorf("invalid Zip URL: %s . Should either be prefixed with file://, http:// or https://", zipURL)
}
filenames, err := Unzip(pathToZip, destination, pathToUnzip)

View File

@@ -1377,21 +1377,21 @@ func TestIsValidProjectDir(t *testing.T) {
devfilePath: "devfile.yaml",
filesToCreate: []string{"file1.yaml"},
dirToCreate: []string{},
expectedError: "Folder %s doesn't contain the devfile used.",
expectedError: "folder %s doesn't contain the devfile used",
},
{
name: "Case 4: Folder contains a hidden file which is not the devfile",
devfilePath: "devfile.yaml",
filesToCreate: []string{".file1.yaml"},
dirToCreate: []string{},
expectedError: "Folder %s doesn't contain the devfile used.",
expectedError: "folder %s doesn't contain the devfile used",
},
{
name: "Case 5: Folder contains devfile.yaml and more files",
devfilePath: "devfile.yaml",
filesToCreate: []string{"devfile.yaml", "file1.yaml", "file2.yaml"},
dirToCreate: []string{},
expectedError: "Folder %s is not empty. It can only contain the devfile used.",
expectedError: "folder %s is not empty. It can only contain the devfile used",
},
}