Skip to content

Commit

Permalink
Hygiene: enabled presets and various linters.
Browse files Browse the repository at this point in the history
Explicitly enabled:
- maintidx
- makezero
- mustag
- nakedret
- nilerr
- nilnil
- noctx
- nolintlint
- nosprintfhostport

Implicitly enabled: all other default linters in the presets.

- fixed `presets` configuration, which wasn't formated properly and
  did not in fact activate the presets
- explicitly disabled some linters (enabled by `presets`) that highlight
  too many issues to resolve in this PR
- fixed host-port issues identified by `nosprintfhostport`
- removed defunct `nolint:revive` pragmas
- fixed format of `//nolint` directives in conformance with `nolintlint`
- added needed `//nolint` directives.
  • Loading branch information
bendory committed Apr 18, 2023
1 parent 50fa5f1 commit 40b15d3
Show file tree
Hide file tree
Showing 23 changed files with 121 additions and 45 deletions.
87 changes: 80 additions & 7 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,16 @@ linters:
- gosec
- gosimple
- govet
- maintidx
- makezero
- misspell
- musttag
- nakedret
- nilerr
- nilnil
- noctx
- nolintlint
- nosprintfhostport
- revive
- thelper
- typecheck
Expand All @@ -46,11 +55,43 @@ linters:
- usestdlibvars
- whitespace
disable:
- cyclop
- dupl
- exhaustruct
- forcetypeassert
- funlen
- gci
- gochecknoglobals
- gochecknoinits
- gocognit
- gocyclo
- godot
- godox
- goerr113
- gofumpt
- gomnd
- gomoddirectives
- ireturn
- lll
- nestif
- nlreturn
- nonamedreturns
- paralleltest
- prealloc
- predeclared
- staticcheck
# Enabling presets means that new linters that we automatically adopt new
# linters that augment a preset. This also opts us in for replacement linters
# when a linter is deprecated.
presets:
- stylecheck
- tagliatelle
- testpackage
- tparallel
- unparam
- varnamelen
- wrapcheck
- wsl
# Enabling presets means that new linters that we automatically adopt new
# linters that augment a preset. This also opts us in for replacement linters
# when a linter is deprecated.
presets:
- bugs
- comment
- complexity
Expand All @@ -68,13 +109,21 @@ output:
uniq-by-line: false
issues:
exclude-rules:
- path: main.go
linters:
- forbidigo
- path: test/build_logs.go
linters:
- typecheck
- path: _test\.go
linters:
- goconst
- dogsled
- errcheck
- goconst
- gosec
- ineffassign
- maintidx
- typecheck
- path: test/pipelinerun_test\.go
linters:
- unused
Expand All @@ -84,24 +133,48 @@ issues:
- path: pkg/pipelinerunmetrics/injection.go
linters:
- containedctx
- path: pkg/pod/pod.go
linters:
- maintidx
- path: pkg/pod/creds_init_test.go
linters:
- containedctx
- path: pkg/reconciler/pipelinerun/pipelinerun.go
linters:
- maintidx
- path: pkg/taskrunmetrics/injection.go
linters:
- containedctx
- path: test/controller.go
linters:
- containedctx
- path: internal/sidecarlogresults/sidecarlogresults\.go
linters:
- musttag
- path: internal/sidecarlogresults/sidecarlogresults_test\.go
linters:
- errchkjson
- path: pkg/apis/pipeline/v1beta1/param_types\.go
linters:
- musttag
- path: pkg/apis/pipeline/v1/param_types\.go
linters:
- musttag
- path: pkg/resolution/resolver/framework/testing/fakecontroller.go
linters:
- contextcheck
- path: pkg/pipelinerunmetrics/metrics.go
linters:
- contextcheck
- path: pkg/reconciler/pipelinerun/pipelinerun.go
linters:
- contextcheck
max-issues-per-linter: 0
max-same-issues: 0
include:
# Enable off-by-default rules for revive requiring that all exported elements have a properly formatted comment.
- EXC0012
- EXC0014
- EXC0012 # https://golangci-lint.run/usage/false-positives/#exc0012
- EXC0014 # https://golangci-lint.run/usage/false-positives/#exc0014
run:
issues-exit-code: 1
build-tags:
Expand Down
2 changes: 1 addition & 1 deletion cmd/entrypoint/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func main() {

if err := e.Go(); err != nil {
breakpointExitPostFile := e.PostFile + breakpointExitSuffix
switch t := err.(type) { // nolint -- checking for multiple types with errors.As is ugly.
switch t := err.(type) { //nolint:errorlint // checking for multiple types with errors.As is ugly.
case skipError:
log.Print("Skipping step because a previous step failed")
os.Exit(1)
Expand Down
1 change: 0 additions & 1 deletion cmd/webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ func newConfigValidationController(name string) func(context.Context, configmap.
}

func newConversionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
// nolint: revive
var (
v1beta1GroupVersion = v1beta1.SchemeGroupVersion.Version
v1GroupVersion = v1.SchemeGroupVersion.Version
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/config/feature_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ const (
disableCredsInitKey = "disable-creds-init"
runningInEnvWithInjectedSidecarsKey = "running-in-environment-with-injected-sidecars"
awaitSidecarReadinessKey = "await-sidecar-readiness"
requireGitSSHSecretKnownHostsKey = "require-git-ssh-secret-known-hosts" // nolint: gosec
requireGitSSHSecretKnownHostsKey = "require-git-ssh-secret-known-hosts" //nolint:gosec
enableTektonOCIBundles = "enable-tekton-oci-bundles"
enableAPIFields = "enable-api-fields"
sendCloudEventsForRuns = "send-cloudevents-for-runs"
Expand Down
2 changes: 0 additions & 2 deletions pkg/apis/pipeline/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@ package pipeline

const (
// PipelineRunControllerName holds the name of the PipelineRun controller
// nolint: revive
PipelineRunControllerName = "PipelineRun"

// PipelineControllerName holds the name of the Pipeline controller
// nolint: revive
PipelineControllerName = "Pipeline"

// TaskRunControllerName holds the name of the TaskRun controller
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ func (tr *TaskRun) GetTimeout(ctx context.Context) time.Duration {
// Use the platform default is no timeout is set
if tr.Spec.Timeout == nil {
defaultTimeout := time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes)
return defaultTimeout * time.Minute
return defaultTimeout * time.Minute //nolint:durationcheck
}
return tr.Spec.Timeout.Duration
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipelineref_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (ref *PipelineRef) Validate(ctx context.Context) (errs *apis.FieldError) {
}
}
}
return
return //nolint:nakedret
}

func validateBundleFeatureFlag(ctx context.Context, featureName string, wantValue bool) *apis.FieldError {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/taskref_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,5 @@ func (ref *TaskRef) Validate(ctx context.Context) (errs *apis.FieldError) {
}
}
}
return
return //nolint:nakedret
}
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ func (tr *TaskRun) GetTimeout(ctx context.Context) time.Duration {
// Use the platform default is no timeout is set
if tr.Spec.Timeout == nil {
defaultTimeout := time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes)
return defaultTimeout * time.Minute
return defaultTimeout * time.Minute //nolint:durationcheck
}
return tr.Spec.Timeout.Duration
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/credentials/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func SortAnnotations(secrets map[string]string, annotationPrefix string) []strin
// /tekton/creds directory is not considered an error.
func CopyCredsToHome(credPaths []string) error {
if info, err := os.Stat(pipeline.CredsDir); err != nil || !info.IsDir() {
return nil
return nil //nolint:nilerr // safe to ignore error; no credentials available to copy
}

homepath, err := homedir.Dir()
Expand Down
10 changes: 5 additions & 5 deletions pkg/pod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
// Superceded by podTemplate envs
if len(implicitEnvVars) > 0 {
for i, s := range stepContainers {
env := append(implicitEnvVars, s.Env...) // nolint:gocritic
env := append(implicitEnvVars, s.Env...) //nolint:gocritic
stepContainers[i].Env = env
}
}
Expand All @@ -235,15 +235,15 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
}
if len(podTemplate.Env) > 0 {
for i, s := range stepContainers {
env := append(s.Env, filteredEnvs...) // nolint:gocritic
env := append(s.Env, filteredEnvs...) //nolint:gocritic
stepContainers[i].Env = env
}
}
// Add env var if hermetic execution was requested & if the alpha API is enabled
if taskRun.Annotations[ExecutionModeAnnotation] == ExecutionModeHermetic && alphaAPIEnabled {
for i, s := range stepContainers {
// Add it at the end so it overrides
env := append(s.Env, corev1.EnvVar{Name: TektonHermeticEnvVar, Value: "1"}) // nolint:gocritic
env := append(s.Env, corev1.EnvVar{Name: TektonHermeticEnvVar, Value: "1"}) //nolint:gocritic
stepContainers[i].Env = env
}
}
Expand Down Expand Up @@ -280,7 +280,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
toAdd = append(toAdd, imp)
}
}
vms := append(s.VolumeMounts, toAdd...) // nolint:gocritic
vms := append(s.VolumeMounts, toAdd...) //nolint:gocritic
stepContainers[i].VolumeMounts = vms
}

Expand All @@ -301,7 +301,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
toAdd = append(toAdd, imp)
}
}
vms := append(s.VolumeMounts, toAdd...) // nolint:gocritic
vms := append(s.VolumeMounts, toAdd...) //nolint:gocritic
sidecarContainers[i].VolumeMounts = vms
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/pod/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ func extractStartedAtTimeFromResults(results []result.RunResult) (*metav1.Time,
return &startedAt, nil
}
}
return nil, nil
return nil, nil //nolint:nilnil // would be more ergonomic to return a sentinel error
}

func extractExitCodeFromResults(results []result.RunResult) (*int32, error) {
Expand All @@ -350,7 +350,7 @@ func extractExitCodeFromResults(results []result.RunResult) (*int32, error) {
return &exitCode, nil
}
}
return nil, nil
return nil, nil //nolint:nilnil // would be more ergonomic to return a sentinel error
}

func updateCompletedTaskRunStatus(logger *zap.SugaredLogger, trs *v1beta1.TaskRunStatus, pod *corev1.Pod) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/pipelineref.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func GetVerifiedPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekt
if err := trustedresources.VerifyPipeline(ctx, p, k8s, refSource, verificationpolicies); err != nil {
// FixMe: the below %v should be %w (and the nolint pragma removed)
// but making that change causes e2e test failures.
return nil, nil, fmt.Errorf("GetVerifiedPipelineFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) // nolint:errorlint
return nil, nil, fmt.Errorf("GetVerifiedPipelineFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) //nolint:errorlint
}
return p, s, nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1944,7 +1944,7 @@ func TestResolvePipelineRun_VerificationFailed(t *testing.T) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.RefSource, error) {
return nil, nil, trustedresources.ErrResourceVerificationFailed
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil }
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil } //nolint:nilnil
pr := v1beta1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
Expand Down Expand Up @@ -2214,7 +2214,7 @@ func TestIsCustomTask(t *testing.T) {
getTask := func(ctx context.Context, name string) (*v1beta1.Task, *v1beta1.RefSource, error) {
return task, nil, nil
}
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil }
getTaskRun := func(name string) (*v1beta1.TaskRun, error) { return nil, nil } //nolint:nilnil
getRun := func(name string) (v1beta1.RunObject, error) { return nil, nil }

for _, tc := range []struct {
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/resources/taskref.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func GetVerifiedTaskFunc(ctx context.Context, k8s kubernetes.Interface, tekton c
refSource = s.URI
}
if err := trustedresources.VerifyTask(ctx, t, k8s, refSource, verificationpolicies); err != nil {
return nil, nil, fmt.Errorf("GetVerifiedTaskFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) // nolint:errorlint
return nil, nil, fmt.Errorf("GetVerifiedTaskFunc failed: %w: %v", trustedresources.ErrResourceVerificationFailed, err) //nolint:errorlint
}
return t, s, nil
}
Expand Down
15 changes: 10 additions & 5 deletions pkg/resolution/resolver/hub/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func (r *Resolver) Resolve(ctx context.Context, params []pipelinev1beta1.Param)
case ArtifactHubType:
url := fmt.Sprintf(r.ArtifactHubURL, paramsMap[ParamKind], paramsMap[ParamCatalog], paramsMap[ParamName], paramsMap[ParamVersion])
resp := artifactHubResponse{}
if err := fetchHubResource(url, &resp); err != nil {
if err := fetchHubResource(ctx, url, &resp); err != nil {
return nil, fmt.Errorf("fail to fetch Artifact Hub resource: %w", err)
}
return &ResolvedHubResource{
Expand All @@ -142,7 +142,7 @@ func (r *Resolver) Resolve(ctx context.Context, params []pipelinev1beta1.Param)
case TektonHubType:
url := fmt.Sprintf(r.TektonHubURL, paramsMap[ParamCatalog], paramsMap[ParamKind], paramsMap[ParamName], paramsMap[ParamVersion])
resp := tektonHubResponse{}
if err := fetchHubResource(url, &resp); err != nil {
if err := fetchHubResource(ctx, url, &resp); err != nil {
return nil, fmt.Errorf("fail to fetch Tekton Hub resource: %w", err)
}
return &ResolvedHubResource{
Expand Down Expand Up @@ -192,11 +192,16 @@ func (r *Resolver) isDisabled(ctx context.Context) bool {
return !cfg.FeatureFlags.EnableHubResolver
}

func fetchHubResource(apiEndpoint string, v interface{}) error {
func fetchHubResource(ctx context.Context, apiEndpoint string, v interface{}) error {
// #nosec G107 -- URL cannot be constant in this case.
resp, err := http.Get(apiEndpoint)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiEndpoint, nil)
if err != nil {
return fmt.Errorf("error requesting resource from Hub: %w", err)
return fmt.Errorf("constructing request: %w", err)
}

resp, err := http.DefaultClient.Do(req)
if err != nil {
return fmt.Errorf("requesting resource from Hub: %w", err)
}
if resp.StatusCode != http.StatusOK {
return fmt.Errorf("requested resource '%s' not found on hub", apiEndpoint)
Expand Down
2 changes: 1 addition & 1 deletion pkg/spire/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func getResultValue(result result.RunResult) (string, error) {
valList = append(valList, aos.ArrayVal...)
return strings.Join(valList, ","), nil
case v1beta1.ParamTypeObject:
keys := make([]string, len(aos.ObjectVal))
keys := make([]string, 0, len(aos.ObjectVal))
for k := range aos.ObjectVal {
keys = append(keys, k)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func GetTaskRunStatusForPipelineTask(ctx context.Context, client versioned.Inter
return nil, err
}
if tr == nil {
return nil, nil
return nil, nil //nolint:nilnil // would be more ergonomic to return a sentinel error
}

return &tr.Status, nil
Expand All @@ -56,7 +56,7 @@ func GetCustomRunStatusForPipelineTask(ctx context.Context, client versioned.Int
return nil, err
}
if r == nil {
return nil, nil
return nil, nil //nolint:nilnil // would be more ergonomic to return a sentinel error
}
runStatus = &r.Status
default:
Expand Down
Loading

0 comments on commit 40b15d3

Please sign in to comment.