Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perform staticcheck in CI for better QA #310

Open
2 tasks
muhlemmer opened this issue Mar 6, 2023 · 3 comments
Open
2 tasks

perform staticcheck in CI for better QA #310

muhlemmer opened this issue Mar 6, 2023 · 3 comments
Labels
auth backend enhancement New feature or request good first issue Good for newcomers

Comments

@muhlemmer
Copy link
Collaborator

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 muhlemmer added enhancement New feature or request backend labels Mar 6, 2023
@muhlemmer
Copy link
Collaborator Author

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

@nibosea
Copy link

nibosea commented Sep 20, 2023

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 muhlemmer added the good first issue Good for newcomers label Sep 20, 2023
@muhlemmer
Copy link
Collaborator Author

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!

@hifabienne hifabienne added the auth label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth backend enhancement New feature or request good first issue Good for newcomers
Projects
Status: 📨 Product Backlog
Development

No branches or pull requests

3 participants