Temporarily disable the complexity-related lint rules

We're intending to gradually improve all of the existing functions that
fail these checks as a separate project from other work, because fixing
for these particular lint rules tends to be too invasive to be safe or
sensible to combine with other work.

Therefore we'll temporarily disable these lints from the main lint run
and add a separate .golangci-complexity.yml that we can use to track our
progress towards eliminating those lint failures without continuing to
litter the code with nolint comments in the meantime.

This also removes all of the existing nolint comments for these linters so
that we can start fresh and review each one as part of our improvement
project.

We'll re-enable these linters (and remove .golangci-complexity.yml) once
each example has either been rewritten to pass the checks or we've
concluded that further decomposition would hurt readability and so added
"nolint" comments back in so we can review whether our lint rules are too
strict once we've got a bunch of examples to consider together.

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This commit is contained in:
Martin Atkins 2025-01-02 16:55:06 -08:00 committed by Christian Mesh
parent f802281c5a
commit ad32bde2ae
15 changed files with 71 additions and 23 deletions

64
.golangci-complexity.yml Normal file
View File

@ -0,0 +1,64 @@
# Copyright (c) The OpenTofu Authors
# SPDX-License-Identifier: MPL-2.0
# Copyright (c) 2023 HashiCorp, Inc.
# SPDX-License-Identifier: MPL-2.0
# This is a temporary variant of .golangci.yml to support our work on improving
# existing functions to pass the code complexity lint rules:
# https://github.com/opentofu/opentofu/issues/2325
#
# If you are working on the code complexity improvement project then you can
# check our progress on the complexity-related lints by running the linters
# as follows:
# golangci-lint run -c .golangci-complexity.yml
#
# Many existing functions were failing multiple linters at once at the start
# of this project, and for as long as that remains true it might be helpful
# to add the --uniq-by-line=false option to review all of the failures at
# once.
#
# This file should be deleted at the same time as we re-enable the five
# complexity-related linters in .golangci.yml.
run:
timeout: 30m
linters-settings:
funlen:
lines: 100
statements: 50
ignore-comments: true
nolintlint:
require-explanation: true
require-specific: true
cyclop:
max-complexity: 20
gocognit:
min-complexity: 50
nestif:
min-complexity: 6
issues:
exclude-rules:
- path: (.+)_test.go
linters:
- funlen
- dupl
- revive
- path: (.+)_test.go
text: "ST1003"
- path: (.+)_test.go
text: "var-naming: don't use underscores in Go names"
linters:
disable-all: true
enable:
- cyclop
- funlen
- gocognit
- gocyclo
- nestif

View File

@ -67,7 +67,7 @@ linters:
- asciicheck
- bidichk
- bodyclose
- cyclop
#- cyclop # (refer to .golangci-complexity.yml)
- dupl
- durationcheck
- errcheck
@ -76,14 +76,14 @@ linters:
- exhaustive
- exportloopref
- forbidigo
- funlen
#- funlen # (refer to .golangci-complexity.yml)
- gocheckcompilerdirectives
- gochecknoglobals
- gochecknoinits
- gocognit
#- gocognit # (refer to .golangci-complexity.yml)
- goconst
- gocritic
- gocyclo
#- gocyclo # (refer to .golangci-complexity.yml)
- goimports
#- gomoddirectives Disabled while we deal with the HCL fork
- gomodguard
@ -98,7 +98,7 @@ linters:
- mnd
- musttag
- nakedret
- nestif
#- nestif # (refer to .golangci-complexity.yml)
- nilerr
- nilnil
- noctx

View File

@ -41,7 +41,6 @@ func (b *Local) LocalRun(ctx context.Context, op *backend.Operation) (*backend.L
return lr, stateMgr, diags
}
//nolint:funlen // Historical function predates our complexity rules
func (b *Local) localRun(ctx context.Context, op *backend.Operation) (*backend.LocalRun, *configload.Snapshot, statemgr.Full, tfdiags.Diagnostics) {
var diags tfdiags.Diagnostics

View File

@ -23,8 +23,6 @@ import (
)
// Context implements backend.Local.
//
//nolint:funlen,nestif // Historical function predates our complexity rules
func (b *Remote) LocalRun(_ context.Context, op *backend.Operation) (*backend.LocalRun, statemgr.Full, tfdiags.Diagnostics) {
var diags tfdiags.Diagnostics
ret := &backend.LocalRun{

View File

@ -23,8 +23,6 @@ import (
)
// LocalRun implements backend.Local
//
//nolint:funlen,nestif // Historical function predates our complexity rules
func (b *Cloud) LocalRun(_ context.Context, op *backend.Operation) (*backend.LocalRun, statemgr.Full, tfdiags.Diagnostics) {
var diags tfdiags.Diagnostics
ret := &backend.LocalRun{

View File

@ -81,7 +81,6 @@ func (c *GraphCommand) Run(args []string) int {
// Load the backend
var b backend.Enhanced
//nolint: nestif // This is inspired by apply:PrepareBackend
if lp, ok := planFile.Local(); ok {
plan, planErr := lp.ReadPlan()
if planErr != nil {

View File

@ -507,7 +507,6 @@ func (runner *TestFileRunner) ExecuteTestFile(ctx context.Context, file *modulet
}
}
//nolint:funlen // Historical function predates our complexity rules
func (runner *TestFileRunner) ExecuteTestRun(ctx context.Context, run *moduletest.Run, file *moduletest.File, state *states.State, config *configs.Config) (*states.State, bool) {
log.Printf("[TRACE] TestFileRunner: executing run block %s/%s", file.Name, run.Name)

View File

@ -133,7 +133,7 @@ type DiagnosticFunctionCall struct {
// NewDiagnostic takes a tfdiags.Diagnostic and a map of configuration sources,
// and returns a Diagnostic struct.
func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string]*hcl.File) *Diagnostic { //nolint:funlen,gocognit,gocyclo,cyclop // TODO(1818): Refactor this function.
func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string]*hcl.File) *Diagnostic {
var sev string
switch diag.Severity() {
case tfdiags.Error:

View File

@ -165,7 +165,6 @@ func (mc *ModuleCall) decodeStaticSource(eval *StaticEvaluator) hcl.Diagnostics
// Decode source field
diags := eval.DecodeExpression(mc.Source, StaticIdentifier{Module: eval.call.addr, Subject: fmt.Sprintf("module.%s.source", mc.Name), DeclRange: mc.Source.Range()}, &mc.SourceAddrRaw)
//nolint:nestif // Keeping this similar to the original decode logic for easy review
if !diags.HasErrors() {
// NOTE: This code was originally executed as part of decodeModuleBlock and is now deferred until we have the config merged and static context built
var err error

View File

@ -872,8 +872,6 @@ func providerIterationIdenticalWarning(blockType, target string, sourceExpr, ins
}
// Compares two for_each statements to see if they are "identical". This is on a best-effort basis to help prevent foot-guns.
//
//nolint:funlen,gocognit,gocyclo,cyclop // just a lot of branches
func providerIterationIdentical(a, b hcl.Expression) bool {
if a == nil && b == nil {
return true

View File

@ -136,7 +136,6 @@ func (base *baseEncryption) decrypt(data []byte, validator func([]byte) error) (
inputData := basedata{}
err := json.Unmarshal(data, &inputData)
//nolint:nestif // bugger off
if len(inputData.Version) == 0 || err != nil {
// Not a valid payload, might be already decrypted
verr := validator(data)

View File

@ -130,7 +130,7 @@ func (run *Run) GetReferences() ([]*addrs.Reference, tfdiags.Diagnostics) {
// diagnostics were generated by custom conditions. OpenTofu adds the
// addrs.CheckRule that generated each diagnostic to the diagnostic itself so we
// can tell which diagnostics can be expected.
func (run *Run) ValidateExpectedFailures(expectedFailures addrs.Map[addrs.Referenceable, bool], sourceRanges addrs.Map[addrs.Referenceable, tfdiags.SourceRange], originals tfdiags.Diagnostics) tfdiags.Diagnostics { //nolint: funlen,gocognit,cyclop // legacy code causes this to trigger, we actually reduced the statements
func (run *Run) ValidateExpectedFailures(expectedFailures addrs.Map[addrs.Referenceable, bool], sourceRanges addrs.Map[addrs.Referenceable, tfdiags.SourceRange], originals tfdiags.Diagnostics) tfdiags.Diagnostics {
var diags tfdiags.Diagnostics
for _, diag := range originals {
if rule, ok := addrs.DiagnosticOriginatesFromCheckRule(diag); ok {

View File

@ -36,8 +36,6 @@ import (
// any other Context method with a different config, because the aforementioned
// modified internal state won't match. Again, this is an architectural wart
// that we'll hopefully resolve in future.
//
//nolint:cyclop,funlen,gocognit,nestif // Historical function predates our complexity rules
func (c *Context) Input(ctx context.Context, config *configs.Config, mode InputMode) tfdiags.Diagnostics {
// This function used to be responsible for more than it is now, so its
// interface is more general than its current functionality requires.

View File

@ -124,8 +124,6 @@ type PlanOpts struct {
// planned so far, which is not safe to apply but could potentially be used
// by the UI layer to give extra context to support understanding of the
// returned error messages.
//
//nolint:cyclop,funlen // Historical function predates our complexity rules
func (c *Context) Plan(ctx context.Context, config *configs.Config, prevRunState *states.State, opts *PlanOpts) (*plans.Plan, tfdiags.Diagnostics) {
defer c.acquireRun("plan")()
var diags tfdiags.Diagnostics

View File

@ -118,7 +118,6 @@ func (n *NodeAbstractResourceInstance) resolveProvider(ctx EvalContext, hasExpan
useStateFallback := false
//nolint:nestif // complexity
if n.ResolvedProvider.KeyExact != nil {
// Pass through from state
n.ResolvedProviderKey = n.ResolvedProvider.KeyExact