Update golangci-lint/goimports configuration

[naemono]

Update goimports settings to consider github.com/elastic/cloud-on-k8s a local prefix in imports section.

This standardizes having

1. stdlib imports at top of import section
2. 3rd party imports as the next section
3. local imports as the final section

[david-kow]

I've read through all changes and all looks good. I was able to find few more files that have more than three groups though:

pkg/controller/association/reconciler.go
pkg/controller/association/conf.go
pkg/controller/association/resources_test.go
pkg/controller/association/controller/beat_es.go
pkg/controller/enterprisesearch/pod_test.go
pkg/controller/kibana/pod_test.go
pkg/controller/kibana/driver_test.go
pkg/controller/common/license/verifier_test.go
pkg/controller/common/keystore/resources_test.go
pkg/controller/elasticsearch/remotecluster/elasticsearch_test.go
pkg/controller/elasticsearch/user/associated_test.go
pkg/controller/elasticsearch/user/user_provided_test.go
pkg/controller/elasticsearch/sset/list_test.go
pkg/controller/remoteca/watches.go
pkg/dev/portforward/pod_forwarder_test.go

I found them using the following invocation and while I have little confidence that it's the best way, maybe we should follow up with including this into make lint? With that and goimports we should cover all cases and keep all the imports neat from now on.

$ find . -name "*\.go" | xargs -I 'file' bash -c 'echo -n "file - " && sed -E -n "/import \(/,/\)/P" file | grep -c "^$"' | grep -v '\(0\|1\|2\)' | cut -f 1 -d " "

[pebrc]

If we go for this new three section import block we should also update the contributing guideline accordingly.

[naemono]

Ok, I've updated the contribution document, as well as fixed the additional files. How would yall like to handle detecting imports that are not valid? Add something to the makefile such as the find command @david-kow is suggesting @pebrc ?

[david-kow]

LGTM! As for adding the check I'd be fine both ways, curious what others think too.

[pebrc]

Why do we need another check? Or put differently why did goimports not catch all cases?

[david-kow]

Why do we need another check? Or put differently why did goimports not catch all cases?

Because when more groups are present it thinks that it's on purpose and fixes them only partially.

[naemono]

From doing some investigation, goimports doesn't handle things perfectly as we'd want it to. See golang/go#20818
Along with this https://go-review.googlesource.com/c/tools/+/116795/
which seems to have been reverted, from what I can tell.

Taking one of the files you noted earlier that wasn't "fixed", from the master branch, and note it isn't shown that it would be "fixed" by goimports (that is, the 2x 3rd party groupings aren't combined.

# -d shows the diff, and note 0 output... 
❯ goimports -d -local github.com/elastic/cloud-on-k8s pkg/controller/association/reconciler.go
❯ 

But, as noted in the golang issue above, there is a fork of goimports that seems to handle this properly

❯ gosimports -d -local github.com/elastic/cloud-on-k8s pkg/controller/association/reconciler.go

diff -u pkg/controller/association/reconciler.go.orig pkg/controller/association/reconciler.go
--- pkg/controller/association/reconciler.go.orig	2021-10-14 09:06:10.000000000 -0500
+++ pkg/controller/association/reconciler.go	2021-10-14 09:06:10.000000000 -0500
@@ -10,6 +10,7 @@
 	"reflect"
 	"time"

+	"github.com/go-logr/logr"
 	"go.elastic.co/apm"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -18,8 +19,6 @@
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"

-	"github.com/go-logr/logr"
-
 	commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
 	esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common"

Is this fork a path we want to go down, or simply work under the assumption that PR reviews + golangci-lint will catch the vast majority of these improper imports?

[naemono]

@david-kow @pebrc any thoughts on the above question? Should we just leave as-is, and merge? lmk, thanks.

[david-kow]

I'd be fine both with merging as is and with adding the script to verify group count.

[pebrc]

I'd be fine both with merging as is

Same.