Reorganize go imports to be more consistent

[reggieriser]

Summary

I've noticed that the order of our go imports has seemed a little random and getting more so over time. Ideally, the imports would be organized as follows (alphabetically within a section):

import (
    [system packages]

    [third-party packages]

    [project packages]
)

However, I think when a blank line is added between imports, goimports won't regroup the grouped imports to be with other similar groups. In this PR, I used the gci utility to do the reorganization which eliminates extra sections and groups like I think we want.

Note that a couple of the commits (first, second) fix a few duplicate imports (either alias vs. no alias or "." vs no ".") that caused issues like gci and goimports fighting against each other (goimports still runs as part of our pre-commit hooks).

I tried to add gci to our golangci-lint list of linters to run. However, gci won't automatically fix the import order right now due to recent changes in the library (see this golangci-lint issue). It will fail if there is an issue with the import order, but the error message makes it less than obvious what's happening (see this gci issue). So turning it on may cause more confusion than it solves at this point.

So for now, I've created a script that uses the gci utility directly (in write mode) and set up pre-commit to run non-generated Go code through it for consistent formatting. That said, I'd rather have this work at the golangci-lint level like our other formatters, so maybe we can move to that at some point when the above issues have been addressed.

To test it, try changing the order of imports on a file or two, do a git add on them, then run:
pre-commit run fix-go-imports

To try it on all files (it shouldn't touch generated files),
pre-commit run -a fix-go-imports

[robot-mymove]

	Warnings
⚠️	Please add the JIRA issue key to the PR title (e.g. MB-123)

Generated by 🚫 dangerJS against 996a043

[felipe-lee]

It'd be great to see something like this. I'll fix imports on files I touch sometimes, but sometimes don't if I know others are in that file too because it'll cause more merge conflicts. If we could have it automated, that would be better. I wonder if instead of using it through golangci-lint, we should use it more directly since it seems to have a write command we could use as an "auto-fix" of sorts.

[reggieriser]

I wonder if instead of using it through golangci-lint, we should use it more directly since it seems to have a write command we could use as an "auto-fix" of sorts.

@felipe-lee I used gci write to fix all the files initially. Since gci with golangci-lint isn't working like we'd want now, I just pushed up a commit that exposes that functionality through a make target: make fix_go_imports. See what you think.

[felipe-lee]

I wonder if instead of using it through golangci-lint, we should use it more directly since it seems to have a write command we could use as an "auto-fix" of sorts.

@felipe-lee I used gci write to fix all the files initially. Since gci with golangci-lint isn't working like we'd want now, I just pushed up a commit that exposes that functionality through a make target: make fix_go_imports. See what you think.

I tried it locally and we'd need to also add an exception for skipping .gopath for people using nix.

.PHONY: fix_go_imports
fix_go_imports: bin/gci
	find . -name '*.go' -not -path "./pkg/gen/*" -not -path "./pkg/assets/*" -not -path "*/*mocks/*" -not -path "./.gopath/*" \
		-exec gci write --Section Standard --Section Default --Section "Prefix(github.com/transcom/mymove)" {} \;

I specifically added -not -path "./.gopath/*"

It works pretty quickly though so that's awesome. I wonder if making it a script would be better so that we can run it with pre-commit still, like we do with the ato-go-linter:

mymove/.pre-commit-config.yaml

Lines 59 to 66 in 996a043

	- repo: local
	hooks:
	- id: ato-go-linter
	name: ato-go-linter
	entry: scripts/pre-commit-go-custom-linter ato-linter
	files: \.go$
	pass_filenames: false
	language: script

mymove/scripts/pre-commit-go-custom-linter

Lines 1 to 11 in 996a043

	#! /usr/bin/env bash
	#
	# Pre-commit hook to run a go custom hook against all go files
	#
	set -eu -o pipefail
	linter="$1";
	go run ./cmd/"$linter"/main.go -- ./...

[reggieriser]

I wonder if making it a script would be better so that we can run it with pre-commit still

Yes, that would be much better! I'm not sure why I wasn't thinking of that before; maybe I got my focus tied up on the golangci-lint integration issues. I've pushed a new commit through that tries to do this. I didn't add the .gopath exclusion here yet since none of the other pre-commit hooks seemed to do it, but try things out in your nix setup and I'll add it if needed. Is this more along the lines of what you were thinking?

[ahobson]

I tried it locally and we'd need to also add an exception for skipping .gopath for people using nix.

I really like this! I wonder if it could be better to just supply a list of allowed top level directories? e.g. we only have go code in cmd and pkg right? And we only need to exclude a few directories from pkg?

[reggieriser]

I wonder if it could be better to just supply a list of allowed top level directories? e.g. we only have go code in cmd and pkg right? And we only need to exclude a few directories from pkg?

I just pushed a change to hopefully do this:

mymove/.pre-commit-config.yaml

Lines 69 to 70 in 63adf48

	files: ^cmd/.*\.go$|^pkg/.*\.go$
	exclude: ^pkg/gen/|^pkg/assets/|/*mocks/

[reggieriser]

I just noticed it's failing in CI now at the pre_test step because it can't find the gci executable there. That should be created by make build_tools now, but I guess that's not run in CI. I'll have to dig into that.

[felipe-lee]

Fwiw, I tried both versions of the pre-commit hook you set up (pointing at all go files vs being explicit about cmd and pkg) and both worked for me and updated the same files (afict anyways...it's a lot of files...)

[reggieriser]

Closing in lieu of #8940.