Introduce golangci config and make help

[bavarianbidi]

This PR add help as make target and also add a .golangci.yml file which will get used by golangci-lint.
I've also tried to address a bunch of linter findings without changing to much of the code itself.

Main motivation for a custom .golangci.yml was the need for the prometheus linter which helps to avoid some kind of wrong metric names.

I've tried to put all fixes from the linter findings into one commit per finding-category to make it more easy to review.

initial output of make lint - all the findings got addressed by dedicated commits:

/home/comario/go/src/github.com/cloudbase/garm/bin/golangci-lint run -v 
database/sql/controller_test.go:24:14: could not import github.com/cloudbase/garm/internal/testing (-: build constraints exclude all Go files in internal/testing) (typecheck)
	garmTesting "github.com/cloudbase/garm/internal/testing"
	            ^
runner/enterprises_test.go:27:14: could not import github.com/cloudbase/garm/internal/testing (-: build constraints exclude all Go files in internal/testing) (typecheck)
	garmTesting "github.com/cloudbase/garm/internal/testing"
	            ^
websocket/client.go:8: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/cloudbase/garm) (gci)

params/requests.go:21: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/cloudbase/garm) (gci)
	commonParams "github.com/cloudbase/garm-provider-common/params"

params/requests.go:23: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/cloudbase/garm) (gci)
	"github.com/cloudbase/garm-provider-common/errors"
params/params.go:290:2: ifElseChain: rewrite if-else to switch statement (gocritic)
	if p.RepoID != "" {
	^
test/integration/e2e/instances.go:106:6: badCond: `instancesCount == int(pool.MinIdleRunners) && instancesCount == len(poolInstances)` condition is suspicious (gocritic)
		if instancesCount == int(pool.MinIdleRunners) && instancesCount == len(poolInstances) {
		   ^
cmd/garm-cli/cmd/pool.go:478:3: ifElseChain: rewrite if-else to switch statement (gocritic)
		if pool.RepoID != "" && pool.RepoName != "" {
		^
cmd/garm-cli/cmd/pool.go:508:2: ifElseChain: rewrite if-else to switch statement (gocritic)
	if pool.RepoID != "" && pool.RepoName != "" {
	^
runner/pool/pool.go:1226:4: assignOp: replace `required = required - delta` with `required -= delta` (gocritic)
			required = required - delta
			^
runner/pool/pool.go:677:5: commentFormatting: put a space between `//` and comment text (gocritic)
				//start the instance
				^
apiserver/routers/routers.go:90:3: commentFormatting: put a space between `//` and comment text (gocritic)
		//prints log and metrics
		^
cmd/garm/main.go:157:3: exitAfterDefer: log.Fatalf will exit, and `defer stop()` will not run (gocritic)
		log.Fatalf("Fetching config: %+v", err)
		^
database/common/common.go:80: database/common/common.go:80: Line contains TODO/BUG/FIXME: "TODO: add filter/pagination" (godox)
	// TODO: add filter/pagination
database/common/common.go:107: database/common/common.go:107: Line contains TODO/BUG/FIXME: "TODO: add filter/pagination" (godox)
	// TODO: add filter/pagination
config/config.go:546: config/config.go:546: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): should we have a ..." (godox)
	// TODO(gabriel-samfira): should we have a minimum TTL?
runner/providers/external/external.go:163: runner/providers/external/external.go:163: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): handle error type..." (godox)
	// TODO(gabriel-samfira): handle error types. Of particular interest is to
auth/auth.go:61: auth/auth.go:61: Line contains TODO/BUG/FIXME: "TODO: make this configurable" (godox)
			// TODO: make this configurable
auth/auth.go:90: auth/auth.go:90: Line contains TODO/BUG/FIXME: "TODO: currently this is the same TTL as ..." (godox)
	// TODO: currently this is the same TTL as the normal Token
auth/auth.go:100: auth/auth.go:100: Line contains TODO/BUG/FIXME: "TODO: make this configurable" (godox)
			// TODO: make this configurable
auth/instance_middleware.go:114: auth/instance_middleware.go:114: Line contains TODO/BUG/FIXME: "TODO: Log error details when authenticat..." (godox)
		// TODO: Log error details when authentication fails
auth/jwt.go:90: auth/jwt.go:90: Line contains TODO/BUG/FIXME: "TODO: Log error details when authenticat..." (godox)
		// TODO: Log error details when authentication fails
runner/pool/enterprise.go:81: runner/pool/enterprise.go:81: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): implement caching" (godox)
	// TODO(gabriel-samfira): implement caching
runner/pool/enterprise.go:132: runner/pool/enterprise.go:132: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): Should we make th..." (godox)
		// TODO(gabriel-samfira): Should we make this configurable?
runner/pool/organization.go:93: runner/pool/organization.go:93: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): implement caching" (godox)
	// TODO(gabriel-samfira): implement caching
runner/pool/organization.go:144: runner/pool/organization.go:144: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): Should we make th..." (godox)
		// TODO(gabriel-samfira): Should we make this configurable?
runner/pool/pool.go:58: runner/pool/pool.go:58: Line contains TODO/BUG/FIXME: "TODO: make this configurable(?)" (godox)
	// TODO: make this configurable(?)
runner/pool/pool.go:1147: runner/pool/pool.go:1147: Line contains TODO/BUG/FIXME: "TODO: should probably allow aditional fi..." (godox)
		// TODO: should probably allow aditional filters to list functions. Would help to filter by date
runner/pool/pool.go:1283: runner/pool/pool.go:1283: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): implement request..." (godox)
			// TODO(gabriel-samfira): implement request throttling.
runner/pool/pool.go:1300: runner/pool/pool.go:1300: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): Incrementing Crea..." (godox)
			// TODO(gabriel-samfira): Incrementing CreateAttempt should be done within a transaction.
runner/pool/pool.go:1509: runner/pool/pool.go:1509: Line contains TODO/BUG/FIXME: "TODO: filter instances by status." (godox)
	// TODO: filter instances by status.
runner/pool/pool.go:1775: runner/pool/pool.go:1775: Line contains TODO/BUG/FIXME: "TODO: Implament a cache? Should we retur..." (godox)
				// TODO: Implament a cache? Should we return here?
runner/pool/pool.go:1785: runner/pool/pool.go:1785: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): create an in-memo..." (godox)
			// TODO(gabriel-samfira): create an in-memory state of existing runners that we can easily
runner/pool/repository.go:100: runner/pool/repository.go:100: Line contains TODO/BUG/FIXME: "TODO(gabriel-samfira): Should we make th..." (godox)
		// TODO(gabriel-samfira): Should we make this configurable?
apiserver/controllers/controllers.go:117: apiserver/controllers/controllers.go:117: Line contains TODO/BUG/FIXME: "TODO: check error type" (godox)
		} else if strings.Contains(err.Error(), "signature") { // TODO: check error type
apiserver/controllers/controllers.go:185: apiserver/controllers/controllers.go:185: Line contains TODO/BUG/FIXME: "TODO (gsamfira): Handle ExpiresAt. Right..." (godox)
	// TODO (gsamfira): Handle ExpiresAt. Right now, if a client uses
cmd/garm-cli/common/common.go:42: File is not `gofumpt`-ed (gofumpt)

cmd/garm-cli/common/common.go:62: File is not `gofumpt`-ed (gofumpt)

cmd/garm-cli/config/home_nix.go:15: File is not `gofumpt`-ed (gofumpt)

auth/instance_middleware.go:30: File is not `goimports`-ed with -local github.com/cloudbase/garm (goimports)
	runnerErrors "github.com/cloudbase/garm-provider-common/errors"
	commonParams "github.com/cloudbase/garm-provider-common/params"
cmd/garm-cli/cmd/init.go:25: File is not `goimports`-ed with -local github.com/cloudbase/garm (goimports)
	apiClientFirstRun "github.com/cloudbase/garm/client/first_run"
	apiClientLogin "github.com/cloudbase/garm/client/login"
cmd/garm-cli/cmd/root.go:21: File is not `goimports`-ed with -local github.com/cloudbase/garm (goimports)

config/config_test.go:28:2: G101: Potential hardcoded credentials (gosec)
	EncryptionPassphrase     = "bocyasicgatEtenOubwonIbsudNutDom"
	^
util/util.go:88:21: G402: TLS MinVersion too low. (gosec)
		TLSClientConfig: &tls.Config{
			RootCAs: roots,
		},
test/integration/e2e/instances.go:57:16: G601: Implicit memory aliasing in for loop. (gosec)
				instance = &i
				           ^
test/integration/e2e/jobs.go:98:11: G601: Implicit memory aliasing in for loop. (gosec)
				job = &j
				      ^
apiserver/routers/routers.go:52:2: G108: Profiling endpoint is automatically exposed on /debug/pprof (gosec)
	_ "net/http/pprof" // Register the pprof handlers
	^
cmd/garm/main.go:244:10: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
	srv := &http.Server{
		Addr: cfg.APIServer.BindAddress(),
		// Pass our instance of gorilla/mux in.
		Handler: handlers.CORS(methodsOk, headersOk, allowedOrigins)(router),
	}
runner/common/pool.go:64:60: `procede` is a misspelling of `proceed` (misspell)
	// received from the provider will be ignored and we will procede to remove the runner from the database.
	                                                          ^
cmd/garm-cli/cmd/credentials.go:37:54: `withing` is a misspelling of `within` (misspell)
which in turn can be used to define pools of runners withing repositories.`,
                                                     ^
cmd/garm-cli/cmd/credentials.go:47:71: `availabe` is a misspelling of `available` (misspell)
			Long:         `List the names of the github personal access tokens availabe to the garm.`,
			                                                                   ^
cmd/garm-cli/cmd/pool.go:384:84: `withing` is a misspelling of `within` (misspell)
	poolListCmd.Flags().StringVarP(&poolOrganization, "org", "o", "", "List all pools withing this organization.")
	                                                                                  ^
cmd/garm-cli/cmd/pool.go:385:89: `withing` is a misspelling of `within` (misspell)
	poolListCmd.Flags().StringVarP(&poolEnterprise, "enterprise", "e", "", "List all pools withing this enterprise.")
	                                                                                       ^
cmd/garm-cli/cmd/repository.go:203:37: `respositories` is a misspelling of `repositories` (misspell)
	Long:         `List all configured respositories that are currently managed.`,
	                                   ^
runner/pool/pool.go:648:23: `dissapeared` is a misspelling of `disappeared` (misspell)
							r.ctx, "runner dissapeared from github",
							               ^
params/params.go:440:11: var-declaration: should omit type []byte from declaration of var rest; it will be inferred from the right-hand side (revive)
	var rest []byte = g.CABundle
	         ^
database/common/common.go:31:44: var-naming: interface method parameter repoId should be repoID (revive)
	CreateRepositoryPool(ctx context.Context, repoId string, param params.CreatePoolParams) (params.Pool, error)
	                                          ^
database/common/common.go:50:46: var-naming: interface method parameter orgId should be orgID (revive)
	CreateOrganizationPool(ctx context.Context, orgId string, param params.CreatePoolParams) (params.Pool, error)
	                                            ^
runner/providers/external/external.go:55:35: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (e *external) validateResult(ctx context.Context, inst commonParams.ProviderInstance) error {
                                  ^
runner/providers/external/external.go:271:63: unused-parameter: parameter 'force' seems to be unused, consider removing or renaming it as _ (revive)
func (e *external) Stop(ctx context.Context, instance string, force bool) error {
                                                              ^
test/integration/e2e/utils.go:8:6: var-naming: func printJsonResponse should be printJSONResponse (revive)
func printJsonResponse(resp interface{}) error {
     ^
test/integration/e2e/client.go:19:2: var-naming: var garmUrl should be garmURL (revive)
	garmUrl, err := url.Parse(baseURL)
	^
test/integration/e2e/pools.go:12:33: var-declaration: should drop = 0 from declaration of var timeWaited; it is the zero value (revive)
	var timeWaited time.Duration = 0
	                               ^
test/integration/e2e/instances.go:13:33: var-declaration: should drop = 0 from declaration of var timeWaited; it is the zero value (revive)
	var timeWaited time.Duration = 0
	                               ^
test/integration/e2e/instances.go:44:33: var-declaration: should drop = 0 from declaration of var timeWaited; it is the zero value (revive)
	var timeWaited time.Duration = 0
	                               ^
test/integration/gh_cleanup/main.go:19:16: var-naming: var ctrlIdFound should be ctrlIDFound (revive)
	controllerID, ctrlIdFound := os.LookupEnv("GARM_CONTROLLER_ID")
	              ^
test/integration/gh_cleanup/main.go:27:11: var-naming: var baseUrlFound should be baseURLFound (revive)
	baseURL, baseUrlFound := os.LookupEnv("GARM_BASE_URL")
	         ^
config/config.go:123:3: increment-decrement: should replace providerNames[provider.Name] += 1 with providerNames[provider.Name]++ (revive)
		providerNames[provider.Name] += 1
		^
cmd/garm-cli/cmd/root.go:63:6: var-naming: func initApiClient should be initAPIClient (revive)
func initApiClient(baseUrl, token string) {
     ^
cmd/garm-cli/cmd/root.go:64:2: var-naming: var baseUrlParsed should be baseURLParsed (revive)
	baseUrlParsed, err := url.Parse(baseUrl)
	^
cmd/garm-cli/cmd/pool.go:456:6: var-naming: var asRawJson should be asRawJSON (revive)
	var asRawJson json.RawMessage
	    ^
runner/pool/enterprise.go:80:44: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (r *enterprise) findRunnerGroupByName(ctx context.Context, name string) (*github.EnterpriseRunnerGroup, error) {
                                           ^
runner/pool/enterprise.go:396:34: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func (r *enterprise) InstallHook(ctx context.Context, req *github.Hook) (params.HookInfo, error) {
                                 ^
runner/pool/enterprise.go:188:1: receiver-naming: receiver name e should be consistent with previous receiver name r for enterprise (revive)
func (e *enterprise) PoolType() params.PoolType {
	return params.EnterprisePool
}
runner/pool/repository.go:94:73: unused-parameter: parameter 'pool' seems to be unused, consider removing or renaming it as _ (revive)
func (r *repository) GetJITConfig(ctx context.Context, instance string, pool params.Pool, labels []string) (jitConfigMap map[string]string, runner *github.Runner, err error) {
                                                                        ^
runner/pool/organization.go:200:1: receiver-naming: receiver name o should be consistent with previous receiver name r for organization (revive)
func (o *organization) PoolType() params.PoolType {
	return params.OrganizationPool
}
runner/pool/pool.go:265:6: var-naming: func jobIdFromLabels should be jobIDFromLabels (revive)
func jobIdFromLabels(labels []string) int64 {
     ^
runner/pool/pool.go:268:4: var-naming: var jobId should be jobID (revive)
			jobId, err := strconv.ParseInt(lbl[len(jobLabelPrefix):], 10, 64)
			^
runner/pool/pool.go:673:11: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive)
			} else {
				slog.InfoContext(
					r.ctx, "instance was found in stopped state; starting",
					"runner_name", dbInstance.Name)
				//start the instance
				if err := provider.Start(r.ctx, dbInstance.ProviderID); err != nil {
					return errors.Wrapf(err, "starting instance %s", dbInstance.ProviderID)
				}
			}
cmd/garm/main.go:101:14: var-declaration: should omit type []io.Writer from declaration of var writers; it will be inferred from the right-hand side (revive)
	var writers []io.Writer = []io.Writer{
	            ^
cmd/garm-cli/cmd/pool.go:535:53: unnecessary conversion (unconvert)
	t.AppendRow(table.Row{"GitHub Runner Group", string(pool.GitHubRunnerGroup)})
	                                                   ^
runner/pool/pool.go:416:37: unnecessary conversion (unconvert)
		switch commonParams.InstanceStatus(instance.Status) {
		                                  ^
runner/pool/pool.go:574:37: unnecessary conversion (unconvert)
		switch commonParams.InstanceStatus(dbInstance.Status) {
		                                  ^
runner/providers/external/external.go:148: unnecessary trailing newline (whitespace)

	}
auth/auth.go:80: unnecessary leading newline (whitespace)
func (a *Authenticator) GetJWTMetricsToken(ctx context.Context) (string, error) {

auth/metrics.go:25: unnecessary leading newline (whitespace)
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

cmd/garm-cli/cmd/enterprise.go:177: unnecessary leading newline (whitespace)
func init() {

[gabriel-samfira]

This is a really nice cleanup! The imports especially is something I've been procrastinating since forever 😄 . Was always looking at them being out of order and was thinking: "not now...later". For 2 years now 😄

[bavarianbidi]

sorry - but i need to do a second round - was running make lint locally all the time - but in the CI, golangci-lint is triggered with --build-tags testing. Will take a look on that

[gabriel-samfira]

No worries. I ran it exactly because in most cases, our local env differs from github and it's good to see where things are at :)

[gabriel-samfira]

Thanks for this!

[gabriel-samfira]

Seems integration tests are failing now: https://github.com/cloudbase/garm/actions/runs/8009664375/job/21878849986

Will investigate soon.