From ad32bde2ae0c2945551eadc749f17808007c6683 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Thu, 2 Jan 2025 16:55:06 -0800
Subject: [PATCH] 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>
---
 .golangci-complexity.yml                      | 64 +++++++++++++++++++
 .golangci.yml                                 | 10 +--
 internal/backend/local/backend_local.go       |  1 -
 internal/backend/remote/backend_context.go    |  2 -
 internal/cloud/backend_context.go             |  2 -
 internal/command/graph.go                     |  1 -
 internal/command/test.go                      |  1 -
 internal/command/views/json/diagnostic.go     |  2 +-
 internal/configs/module_call.go               |  1 -
 internal/configs/provider_validation.go       |  2 -
 internal/encryption/base.go                   |  1 -
 internal/moduletest/run.go                    |  2 +-
 internal/tofu/context_input.go                |  2 -
 internal/tofu/context_plan.go                 |  2 -
 .../tofu/node_resource_abstract_instance.go   |  1 -
 15 files changed, 71 insertions(+), 23 deletions(-)
 create mode 100644 .golangci-complexity.yml

diff --git a/.golangci-complexity.yml b/.golangci-complexity.yml
new file mode 100644
index 0000000000..566456a2e2
--- /dev/null
+++ b/.golangci-complexity.yml
@@ -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
diff --git a/.golangci.yml b/.golangci.yml
index 7d1f1f9347..4917ffb1f8 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -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
diff --git a/internal/backend/local/backend_local.go b/internal/backend/local/backend_local.go
index 2b913df415..07b12d508f 100644
--- a/internal/backend/local/backend_local.go
+++ b/internal/backend/local/backend_local.go
@@ -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
 
diff --git a/internal/backend/remote/backend_context.go b/internal/backend/remote/backend_context.go
index d90ce9abb8..2e2cfd6703 100644
--- a/internal/backend/remote/backend_context.go
+++ b/internal/backend/remote/backend_context.go
@@ -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{
diff --git a/internal/cloud/backend_context.go b/internal/cloud/backend_context.go
index a0901a26c9..3ce4e2c3a1 100644
--- a/internal/cloud/backend_context.go
+++ b/internal/cloud/backend_context.go
@@ -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{
diff --git a/internal/command/graph.go b/internal/command/graph.go
index de8637c7eb..23a4a83892 100644
--- a/internal/command/graph.go
+++ b/internal/command/graph.go
@@ -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 {
diff --git a/internal/command/test.go b/internal/command/test.go
index 5e208ddeff..462c036517 100644
--- a/internal/command/test.go
+++ b/internal/command/test.go
@@ -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)
 
diff --git a/internal/command/views/json/diagnostic.go b/internal/command/views/json/diagnostic.go
index 55c96904a9..e06e97aa3e 100644
--- a/internal/command/views/json/diagnostic.go
+++ b/internal/command/views/json/diagnostic.go
@@ -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:
diff --git a/internal/configs/module_call.go b/internal/configs/module_call.go
index 789c501ae3..c9b1b7289e 100644
--- a/internal/configs/module_call.go
+++ b/internal/configs/module_call.go
@@ -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
diff --git a/internal/configs/provider_validation.go b/internal/configs/provider_validation.go
index db61776988..f39315c9db 100644
--- a/internal/configs/provider_validation.go
+++ b/internal/configs/provider_validation.go
@@ -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
diff --git a/internal/encryption/base.go b/internal/encryption/base.go
index bfb071a570..4d8ff7e525 100644
--- a/internal/encryption/base.go
+++ b/internal/encryption/base.go
@@ -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)
diff --git a/internal/moduletest/run.go b/internal/moduletest/run.go
index c8ffb08055..3ab8691492 100644
--- a/internal/moduletest/run.go
+++ b/internal/moduletest/run.go
@@ -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 {
diff --git a/internal/tofu/context_input.go b/internal/tofu/context_input.go
index 89cfc348f7..fde8bd009c 100644
--- a/internal/tofu/context_input.go
+++ b/internal/tofu/context_input.go
@@ -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.
diff --git a/internal/tofu/context_plan.go b/internal/tofu/context_plan.go
index 586c0c26d5..ed77cbfd51 100644
--- a/internal/tofu/context_plan.go
+++ b/internal/tofu/context_plan.go
@@ -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
diff --git a/internal/tofu/node_resource_abstract_instance.go b/internal/tofu/node_resource_abstract_instance.go
index 44d758c415..ea6cab300a 100644
--- a/internal/tofu/node_resource_abstract_instance.go
+++ b/internal/tofu/node_resource_abstract_instance.go
@@ -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