perform staticcheck in CI for better QA

[muhlemmer]

As a maintainer I want to keep a consistent quality of code. As a reviewer I want help to prevent unused code or difficult to recognize bugs end up in the project. Staticcheck is a tool that helps with this.

Staticcheck is a state of the art linter for the Go programming language. Using static analysis, it finds bugs and performance issues, offers simplifications, and enforces style rules.

By adding this tool the the test suite, pull requests with quality issues will fail to pass.

Acceptance criteria

• [ ] Add the Github action
• [ ] Resolve current quality issues (See details below)

Details

By default it is enabled by the VSCode Golang extension. As a result, it is constantly warning of some inconsistencies in the complete project. This is the status of active warnings as of writing:

On `main` branch

``` pkg/client/client.go:92:2: this value of err is never used (SA4006) pkg/client/client.go:93:2: should check returned error before deferring resp.Body.Close() (SA5001) pkg/client/key.go:9:2: const serviceAccountKey is unused (U1000) pkg/client/key.go:10:2: const applicationKey is unused (U1000) pkg/client/rp/integration_test.go:61:2: this value of err is never used (SA4006) pkg/client/rp/integration_test.go:146:34: should use constant http.StatusFound instead of numeric literal 302 (ST1013) pkg/oidc/token_request.go:206:2: field subjectToken is unused (U1000) pkg/oidc/token_request.go:207:2: field subjectTokenType is unused (U1000) pkg/oidc/token_request.go:208:2: field actorToken is unused (U1000) pkg/oidc/token_request.go:209:2: field actorTokenType is unused (U1000) pkg/oidc/token_request.go:210:2: field resource is unused (U1000) pkg/oidc/token_request.go:211:2: field audience is unused (U1000) pkg/oidc/token_request.go:213:2: field requestedTokenType is unused (U1000) pkg/op/verifier_access_token.go:21:2: field maxAge is unused (U1000) pkg/op/verifier_access_token.go:22:2: field acr is unused (U1000) ```

On `next` branch

``` example/server/storage/storage_dynamic.go:103:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:102:2: the lhs of the comparison gets its value from here and has a concrete type example/server/storage/storage_dynamic.go:133:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:132:2: the lhs of the comparison gets its value from here and has a concrete type example/server/storage/storage_dynamic.go:233:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:232:2: the lhs of the comparison gets its value from here and has a concrete type pkg/client/key.go:9:2: const serviceAccountKey is unused (U1000) pkg/client/key.go:10:2: const applicationKey is unused (U1000) pkg/op/context_test.go:16:3: field r is unused (U1000) pkg/op/context_test.go:17:3: field next is unused (U1000) pkg/op/verifier_access_token.go:21:2: field maxAge is unused (U1000) pkg/op/verifier_access_token.go:22:2: field acr is unused (U1000) ```

[muhlemmer]

CC @adlerhurst - I understood you did something like this for zitadel already?

[nibosea]

example/server/storage/storage_dynamic.go:103:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:102:2: the lhs of the comparison gets its value from here and has a concrete type example/server/storage/storage_dynamic.go:133:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:132:2: the lhs of the comparison gets its value from here and has a concrete type example/server/storage/storage_dynamic.go:233:5: this comparison is always true (SA4023) example/server/storage/storage_dynamic.go:232:2: the lhs of the comparison gets its value from here and has a concrete type

I modified the code to avoid this error as follows. storage_dynamic.go (lines 99~107):

// CreateAccessAndRefreshTokens implements the op.Storage interface
// it will be called for all requests able to return an access and refresh token (Authorization Code Flow, Refresh Token Request)
func (s *multiStorage) CreateAccessAndRefreshTokens(ctx context.Context, request op.TokenRequest, currentRefreshToken string) (accessTokenID string, newRefreshToken string, expiration time.Time, err error) {
    storage, oidcErr := s.storageFromContext(ctx) // storage, err → storage,oidcErr
    if oidcErr != nil {  // err → oidcErr
        return "", "", time.Time{}, err
    }
    return storage.CreateAccessAndRefreshTokens(ctx, request, currentRefreshToken)
}

I am new to Go and do not know if this is a good solution. I would like to know if there is a better solution.

[muhlemmer]

Hi! Welcome to Go! Yes, that is a good suggestion! If you want to get started with open source, feel free to send a PR .

Good luck!

[jkitajima]

Hi!

I am trying to help with this issue but some checks involves changing err return types from *oidc.Error to error interface in files that are auto generated.

One example is the RevokeToken method on pkg/op/mock/storage.mock.go:

func (m *MockStorage) RevokeToken(arg0 context.Context, arg1, arg2, arg3 string) *oidc.Error {
    m.ctrl.T.Helper()
    ret := m.ctrl.Call(m, "RevokeToken", arg0, arg1, arg2, arg3)
    ret0, _ := ret[0].(*oidc.Error)
    return ret0
}

When I run the staticcheck locally it complains:

How should I proceed?

[muhlemmer]

@jkitajima do you have a public branch with the staticcheck addition, so I can have a quick look?

[jkitajima]

Sure!

You can find it here: https://github.com/jkitajima/zitadel-oidc

[muhlemmer]

When you change the interface, you need to rerun the mock generator:

go generate ./pkg/op/mock

However, changing the interface is a breaking change for our semver. If you intend to send a PR, be sure to open it against our next branch instead of main.